search

spacetelescope / stcal

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

JP-3463: Fix poisson variance calculation #255

Closed drlaw1558 closed 6 months ago

drlaw1558 commented 6 months ago

This PR addresses the issue in JP-3463 where the addition of an average dark current to the poisson variance calculation step results in incorrect SCI array values for NIRSpec. This PR adjusts the method of dealing with negative slopes and all-zero variance special cases. Tested against two sets of MIRI data and one set of NIRSpec, and is working as intended both with and without specifying the new parameter.

Resolves JP-3463

Checklist

codecov[bot] commented 6 months ago

Codecov Report

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

Project coverage is 84.99%. Comparing base (af5aefb) to head (acfc8e8). Report is 4 commits behind head on main.

:exclamation: Current head acfc8e8 differs from pull request most recent head 9bab0bc. Consider uploading reports for the commit 9bab0bc to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #255 +/- ## ========================================== - Coverage 85.18% 84.99% -0.19% ========================================== Files 35 35 Lines 6797 6865 +68 ========================================== + Hits 5790 5835 +45 - Misses 1007 1030 +23 ```

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

drlaw1558 commented 6 months ago

Will you add a CI test to test_ramp_fitting.py for non-zero average dark current?

I just added one; I haven't done much with pytest before though so let me know if you think it needs to be tweaked.

kmacdonald-stsci commented 6 months ago

Will you add a CI test to test_ramp_fitting.py for non-zero average dark current?

I just added one; I haven't done much with pytest before though so let me know if you think it needs to be tweaked.

It looks fine. Just something to test what the expected values should be, so it will fail if any changes are made to the way the dark current is used the test will fail. I'll approve this PR now.

hbushouse commented 6 months ago

Started regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1404/

hbushouse commented 6 months ago

Regression test results show what I'm guessing are expected differences in the SCI, ERR, and VAR_POISSON arrays in rate/rateints files, which then propagate to later products. So I think this looks OK.