search

spacetelescope / stcal

https://stcal.readthedocs.io/en/latest/
Other
10 stars 32 forks source link

JP-3638: Flag asymmetrical snowballs #261

Closed mwregan2 closed 4 months ago

mwregan2 commented 5 months ago

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

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.79%. Comparing base (af5aefb) to head (dd386f4). Report is 60 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mwregan2 commented 4 months ago

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.pyhttps://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.pyhttps://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")

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.pyhttps://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)

-

Tests should only write pipeline files. And all files should be written to the pytest directory.

In src/stcal/ramp_fitting/ols_fit.pyhttps://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 GitHubhttps://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*pullrequestreview-2094819060__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y32R9rWbrWl4BT7hAOdcC6SbsycTHQENRtm8mAqxT62_RBtAbFBP_Kt7CPnwPv0rAw8XLEAwee88Ey9nWxoHvXs$, or unsubscribehttps://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 commented 4 months ago

Regtest started at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1496

hbushouse commented 4 months ago

@nden Who's on the hook from Roman these days to run romancal tests for this PR?

hbushouse commented 4 months ago

@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?

mwregan2 commented 4 months ago

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)

@mwregan2https://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 GitHubhttps://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*issuecomment-2148745660__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc594dMBBM$, or unsubscribehttps://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: @.***>

mwregan2 commented 4 months ago

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)

@mwregan2https://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 GitHubhttps://urldefense.com/v3/__https:/github.com/spacetelescope/stcal/pull/261*issuecomment-2148745660__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!y8SY-uTNr5mv4-PuZkaRLinYtLEQzvkk8N-9vG_WKJU_FLHYScofnYikorerBxjLp2gMF5J1NjmVuTc594dMBBM$, or unsubscribehttps://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: @.***>

mwregan2 commented 4 months ago

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.

mwregan2 commented 4 months ago

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.