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: Add average_dark_current handling to dark_current step #8302

Merged
merged 23 commits into from
Mar 13, 2024

Conversation

tapastro
Copy link
Contributor

@tapastro tapastro commented Feb 22, 2024

Resolves JP-3463

Closes #8071

This PR adds pipeline handling of new datamodel quantities for the average dark current. This can be specified either by the user, as a scalar quantity, or in the dark reference file, as a scalar quantity or a 2D array matching the dark reference file size. If the user provides a value, it will override any dark reference value. If a user wishes to provide a 2D array, it will be left to them to generate a custom dark.

This quantity has been/wiill be added to the RampModel in spacetelescope/stdatamodels#265; it will be stored in the RampModel for use in the ramp_fitting step.

NOTE: This PR is labelled WIP until the stdatamodels PR linked above is merged.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 53.15%. Comparing base (4cc0ac1) to head (fd5406f).
Report is 22 commits behind head on master.

Files Patch % Lines
jwst/dark_current/dark_current_step.py 70.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8302       +/-   ##
===========================================
- Coverage   75.15%   53.15%   -22.01%     
===========================================
  Files         470      389       -81     
  Lines       38604    38442      -162     
===========================================
- Hits        29014    20434     -8580     
- Misses       9590    18008     +8418     
Flag Coverage Δ
nightly ?

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

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.

Code updates look good. Just a couple questions about the new unit test.

jwst/ramp_fitting/tests/test_ramp_fit.py Outdated Show resolved Hide resolved
jwst/ramp_fitting/tests/test_ramp_fit.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

@tapastro If you can address my couple of review questions I'd be willing to approve. Unless you think it needs more work.

@hbushouse hbushouse requested a review from emolter March 8, 2024 21:20
@tapastro
Copy link
Contributor Author

tapastro commented Mar 8, 2024

I just need to spend some time determining if that test provides any useful coverage, or if I should get another regression test parametrization in that sets the dark current to ensure it's doing someting. I'll work on better testing and get back to you next week.

@tapastro tapastro changed the title WIP: JP-3463: Add average_dark_current handling to dark_current step JP-3463: Add average_dark_current handling to dark_current step Mar 12, 2024
@tapastro
Copy link
Contributor Author

Regression tests here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1301/

I added a test to ensure data is tested with a non-zero average dark current specified. This test can be removed if/when darks are delivered with non-zero values.

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

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

LGTM, just found one small typo in docs

docs/jwst/dark_current/arguments.rst Outdated Show resolved Hide resolved
Co-authored-by: Ned Molter <emolter@users.noreply.github.com>
@hbushouse
Copy link
Collaborator

The 1301 regtest run shows only the expected differences in products created during Detector1 processing (before ramp fitting), which now contain either the extra "AVDRKCUR" image extension or the new "AVDRKCUR" keyword. So that looks good.

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.

Yay, the docs have build correctly and the regtest results look good. I hereby approve this PR.

@hbushouse hbushouse merged commit a4302e8 into spacetelescope:master Mar 13, 2024
27 of 29 checks passed
@braingram
Copy link
Collaborator

I have to say I'm a little sad the message for this commit didn't make it into main due to the squash :)
fd5406f
It describes how I often feel.

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.

Ramp Fit Step Uncertainty is missing the Poisson noise from the Dark Current
5 participants