Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-3463: Fix poisson variance calculation #255

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

drlaw1558
Copy link
Contributor

@drlaw1558 drlaw1558 commented Apr 19, 2024

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

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@drlaw1558 drlaw1558 requested a review from a team as a code owner April 19, 2024 16:03
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@drlaw1558
Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment on the change log entry. Will start a regression test run using this PR branch.

CHANGES.rst Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

@hbushouse
Copy link
Collaborator

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.

@hbushouse hbushouse merged commit fa14c81 into spacetelescope:main Apr 23, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants