JP-3638: Flag asymmetrical snowballs #261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
- Coverage 85.18% 83.79% -1.40%
==========================================
Files 35 36 +1
Lines 6797 7009 +212
==========================================
+ Hits 5790 5873 +83
- Misses 1007 1136 +129 ☔ View full report in Codecov by Sentry. |
…gan2/stcal into flag_asymmetrical_snowballs
kmacdonald-stsci
requested changes
Jun 3, 2024
|
Ken,
I’m getting a test failure on the test
test / test_downstream / py311-jwst-cov-xdist (ubuntu-latest) (pull_request)
I see two “F” values in the results but I’ve never been able to figure out how to map the “F”values to the individual test. Is there something that would tell me what failed?
Thanks
Mike
From: Ken MacDonald ***@***.***>
Reply-To: spacetelescope/stcal ***@***.***>
Date: Monday, June 3, 2024 at 4:52 PM
To: spacetelescope/stcal ***@***.***>
Cc: Michael Regan ***@***.***>, Author ***@***.***>
Subject: Re: [spacetelescope/stcal] Flag asymmetrical snowballs (PR #261)
@kmacdonald-stsci requested changes on this pull request.
The pipeline should only output expected output files. All output files should be written to the pytest temporary folder. The imports in the ols_fit.py file should not be commented out.
________________________________
In src/stcal/jump/jump.py<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*discussion_r1625028768__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9n8gnJbsU$>:
@@ -465,6 +465,7 @@ def detect_jumps(
# This is the flag that controls the flagging of snowballs.
if expand_large_events:
+# fits.writeto("ingdq.fits", gdq)
Should the pipeline be outputting files that aren't expected?
________________________________
In tests/test_jump.py<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*discussion_r1625030946__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nF1McL1k$>:
@@ -545,10 +550,11 @@ def test_inside_ellipes5():
@pytest.mark.skip(" used for local testing")
def test_flag_persist_groups():
# gdq = fits.getdata("persistgdq.fits")
- gdq = np.zeros(shape=(2,2,2,2))
- print(gdq.shape[0])
- gdq = gdq[:, 0:10, :, :]
- total_snowballs = flag_large_events(
+ gdq = fits.getdata("../../../notebooks/ingdq.fits")
This should not be here. The 'ingdq.fits' file should not be outputted by the pipeline. And for testing, all files should be written to the temporary directory.
________________________________
In tests/test_jump.py<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*discussion_r1625032060__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nizLD5pU$>:
@@ -561,7 +567,7 @@ def test_flag_persist_groups():
sat_expand=1.1,
mask_persist_grps_next_int=True,
persist_grps_flagged=0)
-
+ fits.writeto("../../../notebooks/gdqout.fits", gdqout, overwrite=True)
Tests should only write pipeline files. And all files should be written to the pytest directory.
________________________________
In src/stcal/ramp_fitting/ols_fit.py<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*discussion_r1625034818__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nKMOsKFU$>:
@@ -9,7 +9,7 @@
import numpy as np
…-from .slope_fitter import ols_slope_fitter # c extension
+#from .slope_fitter import ols_slope_fitter # c extension
Why is this commented out?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*pullrequestreview-2094819060__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nWxoHvXs$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AF6KJLTIWDPYXBTRLBNESELZFTJP3AVCNFSM6AAAAABIWVBG3OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJUHAYTSMBWGA__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nFPdOjxM$>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
hbushouse
reviewed
Jun 4, 2024
|
@nden Who's on the hook from Roman these days to run romancal tests for this PR? |
|
@mwregan2 The regression test run shows differences in the jump step results for a NIRISS and NIRSpec exposure. For the NIRISS data, the number of extended CRs flagged, as recorded in the EXTNCRS keyword, went up by a factor of 10 compared to before. In the NIRSpec exposure it went up by a factor of 2. Are these large increases expected due to the changes here and are they reasonable? |
|
I can see getting a factor of 2 as a minimum. I’ve look at a sample of long exposures and see what count rate I get.
From: Howard Bushouse ***@***.***>
Reply-To: spacetelescope/stcal ***@***.***>
Date: Tuesday, June 4, 2024 at 10:33 PM
To: spacetelescope/stcal ***@***.***>
Cc: Michael Regan ***@***.***>, Mention ***@***.***>
Subject: Re: [spacetelescope/stcal] Flag asymmetrical snowballs (PR #261)
@mwregan2<https://urldefense.com/v3/__https:/github.com/mwregan2__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc5gWmu09U$> The regression test run shows differences in the jump step results for a NIRISS and NIRSpec exposure. For the NIRISS data, the number of extended CRs flagged, as recorded in the EXTNCRS keyword, went up by a factor of 10 compared to before. In the NIRSpec exposure it went up by a factor of 2. Are these large increases expected due to the changes here and are they reasonable?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*issuecomment-2148745660__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc594dMBBM$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AF6KJLVR6VPKSQGYPY2PWD3ZFZ2GRAVCNFSM6AAAAABIWVBG3OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYG42DKNRWGA__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc5gp5U7lM$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I looked at four 30 min NIRSpec observations. The snowball count with up by 50% for all four of them. That will affect anything that’s looking at the variance of a background or some other region.
From: Michael Regan ***@***.***>
Date: Wednesday, June 5, 2024 at 9:38 AM
To: spacetelescope/stcal ***@***.***>, spacetelescope/stcal ***@***.***>
Cc: Mention ***@***.***>
Subject: Re: [spacetelescope/stcal] Flag asymmetrical snowballs (PR #261)
I can see getting a factor of 2 as a minimum. I’ve look at a sample of long exposures and see what count rate I get.
From: Howard Bushouse ***@***.***>
Reply-To: spacetelescope/stcal ***@***.***>
Date: Tuesday, June 4, 2024 at 10:33 PM
To: spacetelescope/stcal ***@***.***>
Cc: Michael Regan ***@***.***>, Mention ***@***.***>
Subject: Re: [spacetelescope/stcal] Flag asymmetrical snowballs (PR #261)
@mwregan2<https://urldefense.com/v3/__https:/github.com/mwregan2__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc5gWmu09U$> The regression test run shows differences in the jump step results for a NIRISS and NIRSpec exposure. For the NIRISS data, the number of extended CRs flagged, as recorded in the EXTNCRS keyword, went up by a factor of 10 compared to before. In the NIRSpec exposure it went up by a factor of 2. Are these large increases expected due to the changes here and are they reasonable?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*issuecomment-2148745660__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc594dMBBM$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AF6KJLVR6VPKSQGYPY2PWD3ZFZ2GRAVCNFSM6AAAAABIWVBG3OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYG42DKNRWGA__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc5gp5U7lM$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I’ve figured out the cause of the large increase in the NIRISS Snowballs with this PR. The current NIRISS parameters are very conservative and only detect the medium to large snowballs. This causes the step to miss most of the small snowballs which is what most of them are.
I talked with David and he recommended I run a subset of the new regression tests for NIRISS exposures and compare with and without the PR.
I’m working on that now.
|
hbushouse
changed the title
hbushouse
reviewed
Jun 6, 2024
There was a problem hiding this comment.
Based on subsequent discussions regarding the large increase in snowball flagging for NIRISS images, it's been concluded that those are OK and actually desirable.
So my final question before approving is to ask whether this change should result in any updates in the docs anywhere?
|
I should update the report to mention this tweak to the algorithm. It really isn’t a fundamental change. It would just be nice to show an example of the offset saturation.
|
kmacdonald-stsci
approved these changes
Jun 6, 2024
hbushouse
approved these changes
Jun 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves JP-3638
Closes spacetelescope/jwst#8516
This PR addresses the problem of asymmetrical snowballs not getting removed from NIR exposures. These are a sub-population of snowballs where the incoming particle leads a trail of pixels that are flagged as jump and then trigger a circular snowball with a saturated core. Because the current code requires that the center of the ellipse fitted to the pixel flagged as jump have a saturated pixel, these snowballs do not get recognized as a snowball and do not get any increase in the jump ellipse.
The change was to just remove the requirement that the central pixel be saturated and change the requirement to be that there needs to be saturation within a circle with a radius of the major axis of the ellipse centered on the center of the ellipse.
Checklist
CHANGES.rst(either inBug FixesorChanges to API)