spacetelescope / stcal

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

RCAL-618: Add 'read_pattern' argument to saturation flagging. #188

Closed schlafly closed 1 year ago

schlafly commented 1 year ago

This PR adds a new 'read_pattern' argument to the saturation flagging step.

Resolves RCAL-618

In concert with https://github.com/spacetelescope/romancal/pull/836, this PR changes the saturation flagging to depend on the read pattern in use---the pattern by which reads are allocated to resultants, or equivalently frames are allocated to groups. The idea is that the Roman saturation reference file gives the value at which the detector saturates, but resultants/groups are usually averages over several frames/reads, and so simply checking if the resultant is over the threshold isn't adequate to flag all resultants containing at least some saturated reads.

The solution is to adjust the saturation threshold group-by-group by a "dilution factor" that is mean time in each group divided by the time of the last frame in that group.

This PR adds a new argument read_pattern to flag_saturated_pixels to enable that adjustment. When None, the original behavior is retained. Some other changes to the handling of NaN and NO_SAT_CHECK pixels are needed so that the artificial saturation thresholds for these pixels aren't affected by the dilution factor.

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06% :tada:

Comparison is base (1bc4fce) 82.25% compared to head (cb068ba) 82.31%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #188 +/- ## ========================================== + Coverage 82.25% 82.31% +0.06% ========================================== Files 29 29 Lines 5533 5554 +21 ========================================== + Hits 4551 4572 +21 Misses 982 982 ``` | [Files Changed](https://app.codecov.io/gh/spacetelescope/stcal/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | |---|---|---| | [src/stcal/saturation/saturation.py](https://app.codecov.io/gh/spacetelescope/stcal/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-c3JjL3N0Y2FsL3NhdHVyYXRpb24vc2F0dXJhdGlvbi5weQ==) | `98.36% <100.00%> (+0.14%)` | :arrow_up: | | [tests/test\_saturation.py](https://app.codecov.io/gh/spacetelescope/stcal/pull/188?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-dGVzdHMvdGVzdF9zYXR1cmF0aW9uLnB5) | `100.00% <100.00%> (ø)` | |

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

schlafly commented 1 year ago

I think this PR is good to go. Some tests are failing due to tox configuration issues, but I don't think those are related to this PR. I didn't see relevant documentation for the stcal saturation step to update; if there are docs there that I should tackle please let me know. This PR is needed for Roman to account for the varying number of frames per group and accordingly the varying number of unsaturated frames that may be averaged with saturated frames when identifying saturated groups.

This is my first stcal PR, so please let me know what I'm missing!

schlafly commented 1 year ago

I ran the jwst regression tests here, which passed except for this one: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/873/testReport/jwst.regtest/test_associations_sdp_pools/_stable_deps__test_against_standard_jw00016_20230331t130733_pool_/ which appears to be the result of ongoing work in other PRs.
https://github.com/spacetelescope/jwst/commit/6a62c0d7f0ba9cf89d992a752eec15ff969392bb

tox is failing to find test-oldestdeps-cov-xdist, test-jwst-cov-xdist and test-romancal-cov-xdist, which looks to me to be due to the recent tox update and the absence of these names from this list https://github.com/spacetelescope/stcal/blob/main/tox.ini#L2-L7

I'm happy to address that as part of this PR or defer that.

schlafly commented 1 year ago

@hbushouse , @ddavis-stsci , could I get sign off that we can go ahead and merge this without affecting romancal / jwst regression tests? I've run the JWST regression tests above, which look good modulo a current failing JWST association regtest that seems to be due to ongoing work unrelated to this PR. The intention is for this PR to preserve the original behavior when "read_pattern" is not set, and so neither JWST nor Roman should be affected by this PR (until we eventually add that argument into romancal). There are also tox issues that are unrelated to this PR but I can try to clean those up here if desired. Thanks!

schlafly commented 1 year ago

Thanks. I'm blocked from merging this myself until the tox config issues are resolved, as far as I can tell. So I think I need someone else to merge? Thanks!

hbushouse commented 1 year ago

Thanks. I'm blocked from merging this myself until the tox config issues are resolved, as far as I can tell. So I think I need someone else to merge? Thanks!

I can do it. I have uber-github permission (at least in this repo).