Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCAL-862 Refactor L3 Injection #132

Merged
merged 11 commits into from
Aug 6, 2024

Conversation

PaulHuwe
Copy link
Collaborator

@PaulHuwe PaulHuwe commented Jul 7, 2024

This PR greatly reduces the code (and loop) duplication of level 3 source injection. Updates were made to the image source injection and the test. The l3 test was moved from test_image to test l3. Support code was added to wcs and parameters.

NOTE: The l3 and l3_test import blocks are large due to other PRs happening simultaneously.

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 63.51351% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (657c83b) to head (d4af8fd).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
romanisim/util.py 4.00% 24 Missing ⚠️
romanisim/wcs.py 71.42% 2 Missing ⚠️
romanisim/l3.py 97.05% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   89.73%   89.95%   +0.22%     
==========================================
  Files          17       17              
  Lines        1743     1792      +49     
==========================================
+ Hits         1564     1612      +48     
- Misses        179      180       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

romanisim/image.py Outdated Show resolved Hide resolved
romanisim/l3.py Outdated Show resolved Hide resolved
romanisim/l3.py Outdated Show resolved Hide resolved
romanisim/parameters.py Show resolved Hide resolved
romanisim/tests/test_l3.py Outdated Show resolved Hide resolved
method='phot', rng=rng)
else:
try:
stamp = final.drawImage(center=image_pos,
wcs=image.wcs.local(image_pos))
wcs=pwcs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably around here need a:
if add_noise:
stamp.addNoise(galsim.PoissonNoise(rng))
to get around that issue we were discussing that noise is only added in photon-shooting / chromatic mode by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was saving that for RCAL-863.

@PaulHuwe PaulHuwe marked this pull request as ready for review July 10, 2024 15:57
@PaulHuwe PaulHuwe requested a review from a team as a code owner July 10, 2024 15:57
romanisim/image.py Outdated Show resolved Hide resolved
romanisim/image.py Outdated Show resolved Hide resolved
romanisim/l3.py Outdated Show resolved Hide resolved
romanisim/l3.py Outdated Show resolved Hide resolved
romanisim/parameters.py Show resolved Hide resolved
@PaulHuwe PaulHuwe requested a review from schlafly July 24, 2024 20:42
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think the flux_to_counts_factor is missing the factor of abflux now, though; we should reflect a bit about units.

Since they're providing a source_cat the input units are ~required to be in maggies. The ultimate flux_to_counts_factor needs to then be abflux * exptime. Then there's the denominator, now convtimes, that needs to be counts / (target units), so 1 / exptime / (MJy / sr / (count / s)).

The check should probably be that the sum of the output image times the pixel size divided by 3631 is equal to the flux after conversion to Janskies, if that makes sense?

romanisim/parameters.py Show resolved Hide resolved
romanisim/l3.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very minor suggestions inline, but good to go, thanks!

romanisim/parameters.py Show resolved Hide resolved
romanisim/wcs.py Outdated Show resolved Hide resolved
@PaulHuwe PaulHuwe merged commit 12ba182 into spacetelescope:main Aug 6, 2024
16 of 22 checks passed
@PaulHuwe PaulHuwe deleted the RCAL-862_RefactorL3Injection branch August 6, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants