Closed braingram closed 3 months ago
Attention: Patch coverage is 66.66667%
with 2 lines
in your changes missing coverage. Please review.
Project coverage is 79.30%. Comparing base (
79d3a30
) to head (b923030
). Report is 193 commits behind head on main.
Files with missing lines | Patch % | Lines |
---|---|---|
romancal/resample/resample.py | 66.66% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This looks fine to me, but @mairanteodoro and you should discuss, and I suspect that there are related unit tests that will need updating.
Thanks @schlafly!
@mairanteodoro and I have a meeting tomorrow to discuss the sky subtraction (which is partially handled in this PR and partially in https://github.com/spacetelescope/romancal/pull/1233). After updating the flag_outlier
unit test for this PR I don't think the sky subtraction was the only issue so there are a number of changes in this PR.
I'll open this PR for review once the unit and regtests finish (I expect at least the mosaic
regtest to fail and any others that use outlier detection).
Presumably the regtests change meaningfully with this PR; would you mind including something like the new image, old image, and the difference? Thank you!
Presumably the regtests change meaningfully with this PR; would you mind including something like the new image, old image, and the difference? Thank you!
I think the only regtest that is impacted is the mosaic pipeline test. This uses only 3 images and produces very minor differences in the number of detected outliers. With main:
2024-06-04 16:06:53,489 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 6373 (0.04%)
2024-06-04 16:06:54,670 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 11289 (0.07%)
2024-06-04 16:06:55,898 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 6441 (0.04%)
with this PR:
2024-06-04 16:01:20,422 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 6144 (0.04%)
2024-06-04 16:01:21,795 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 15462 (0.09%)
2024-06-04 16:01:22,888 - stpipe.MosaicPipeline.outlier_detection - INFO - New pixels flagged as outliers: 6151 (0.04%)
Overall I don't think the output change is very meaningful given the current regression tests.
Here is the stpreview by <fn> 16 16
output for the truth file:
and the new output with this PR:
I agree that I can't see anything in the preview images; the stars are too small and just look like hot pixels. I don't actually think the L2 files are that problematic; some of the structure in the background is from the non-linearity reference files having issues. The spatial gradient is a little unexpected but is probably very low amplitude. By eye it doesn't look to me like we're masking all of the stars, for example, but have you actually looked at the generated outlier masks?
I agree that I can't see anything in the preview images; the stars are too small and just look like hot pixels. I don't actually think the L2 files are that problematic; some of the structure in the background is from the non-linearity reference files having issues. The spatial gradient is a little unexpected but is probably very low amplitude. By eye it doesn't look to me like we're masking all of the stars, for example, but have you actually looked at the generated outlier masks?
I looked at the outlier masks and there aren't large differences. Since main has seen some changes since I made those comparisons they'll need to be updated. The mosaic test only uses 3 images so the median across groups (many_to_many) vs the drizzled combinations of all groups (many_to_one) produces similar "median" data.
@schlafly Is there more you'd like to see before merging?
I re-ran the regtests here: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-Developers-Pull-Requests/detail/Roman-Developers-Pull-Requests/816/tests
Looking at the output image (a cropped region [200:800, 400:1000]
) the truth file shows the following (log scaled):
with this PR the image is similar but wih a few less CRs:
@schlafly Is there anything else you'd like to see for this PR?
No, thanks, I thought I approved, thank you!
This PR changes outlier detection to use
many_to_many
when resampling during median calculation. This PR produces 1 expected difference in regression test results. https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/805/With the current code on main the
test_level3_mos_pipeline
test runs an association with 3 models (each from a different exposure) through the mosaic pipeline. Setting a breakpoint just prior to the call tocreate_median
: https://github.com/spacetelescope/romancal/blob/ed6187ffa790fc6a37dd96f7365ba1ef8261fa74/romancal/outlier_detection/outlier_detection.py#L108-L111 we can see thatlen(drizzled_models) == 1
. This conflict with the algorithm description in the docs that states:Looking at the outlier detection unit tests I don't see one that both:
resample_data=True
Changing test_outlier_do_detection_find_outliers (which introduces and detects outliers) to use resampling is insufficient to show the impact of this PR. Without this PR, the introduced CRs (value=1E5) and 'drizzled' together with empty pixels from the other image (value=0). This produces a single
drizzled_model
with a value of 5E4 for each PR. The median (N=1) produces the same value, when blotted back to the input image wcs the 5E4 values are far enough below the 1E5 values of the CRs to allow them to be detected. This is true for any value used for the CR (since the input images are noiseless with all 0 error).Furthermore the test appears to be checking that all introduces CRs (even those introduced in
img2
) are flagged inimg1
: https://github.com/spacetelescope/romancal/blob/e559890ee4679835b11e98e17f33ac819733176d/romancal/outlier_detection/tests/test_outlier_detection.py#L253 returns 10 flagged CRs:whereas only 5 were introduced into
img1
: https://github.com/spacetelescope/romancal/blob/e559890ee4679835b11e98e17f33ac819733176d/romancal/outlier_detection/tests/test_outlier_detection.py#L204-L206Switching single (so that now
many_to_many
is used) revealed a few other issues included:The updated unit test in this PR uses 3 images with the first 2 having CRs, each having 1 "source" and checks that:
Checklist
CHANGES.rst
under the corresponding subsection