jwst icon indicating copy to clipboard operation
jwst copied to clipboard

WIP: JP-3463: Add average_dark_current handling to dark_current step

Open tapastro opened this issue 1 year ago • 3 comments

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 https://github.com/spacetelescope/stdatamodels/pull/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

  • [x] added entry in CHANGES.rst within the relevant release section
  • [ ] updated or added relevant tests
  • [x] updated relevant documentation
  • [x] added relevant milestone
  • [x] 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

tapastro avatar Feb 22 '24 14:02 tapastro

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 :warning:
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 ?

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

codecov[bot] avatar Feb 29 '24 16:02 codecov[bot]

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

hbushouse avatar Mar 08 '24 21:03 hbushouse

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 avatar Mar 08 '24 21:03 tapastro

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.

tapastro avatar Mar 12 '24 19:03 tapastro

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.

hbushouse avatar Mar 13 '24 12:03 hbushouse

I have to say I'm a little sad the message for this commit didn't make it into main due to the squash :) https://github.com/spacetelescope/jwst/pull/8302/commits/fd5406fedb7b56617790a268f67c4d43cb57a572 It describes how I often feel.

braingram avatar Mar 13 '24 18:03 braingram