8000 JP-4024: Set negative NIRSpec flat fields to NaN and DNU by hayescr · Pull Request #9522 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-4024: Set negative NIRSpec flat fields to NaN and DNU #9522

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

Merged
merged 7 commits into from
Jun 13, 2025

Conversation

hayescr
Copy link
Contributor
@hayescr hayescr commented Jun 6, 2025

Resolves JP-4024

This PR updates the flat_field behavior for handling negative flat field values for NIRSpec data. Instead of setting the flat field to 1 for any pixels with a negative combined flat, this PR sets the flat field to NaN and flags the pixels with DNU (and adds the remaining bad flat flags). NIRSpec flats include flux calibration (sitting around a value of order ~ 1e10) so a flat field value of 1 is not a good fall back for the NIRSpec combined flat field. The previous behavior was resulting in significant outliers in some NIRSpec data, so it would be better to set any such pixels to NaN/DNU.

From local testing I expect that this will change a small number of outlier pixels to NaN in spec2 and spec3/tso3 products across all NIRSpec modes (for an example IFU data set this was on the order of two dozen pixels) but will also update the DQ flags a larger number of pixels that were already marked as DNU for other reasons (e.g., dead pixels for which the flat fields may be negative). Since this only updates the NIRSpec specific flat field functions it shouldn't affect any other instruments.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

Copy link
codecov bot commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.90%. Comparing base (8e754a0) to head (78c0ff3).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
jwst/flatfield/flat_field.py 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9522      +/-   ##
==========================================
- Coverage   78.90%   78.90%   -0.01%     
==========================================
  Files         36
8000
5      365              
  Lines       37213    37212       -1     
==========================================
- Hits        29363    29362       -1     
  Misses       7850     7850              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator
@melanieclarke melanieclarke 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 a couple questions about whether we should be using NaN instead of 1.0 in other places. One below, and the other:
Since the cal factor is set in the f-flat, should the fore_optics_flat also use NaN for bad values? I think those shouldn't leak through, since DQ is set for those pixels, but there's no reason to set a definitely wrong value there.

@melanieclarke melanieclarke added this to the Build 12.0 milestone Jun 12, 2025
Co-authored-by: Melanie Clarke <mclarke@stsci.edu>
@hayescr
Copy link
Contributor Author
hayescr commented Jun 12, 2025

LGTM, just a couple questions about whether we should be using NaN instead of 1.0 in other places. One below, and the other: Since the cal factor is set in the f-flat, should the fore_optics_flat also use NaN for bad values? I think those shouldn't leak through, since DQ is set for those pixels, but there's no reason to set a definitely wrong value there.

Good point, I'll track those down and set them to NaN as well, no sense in using a wrong default fill value (especially if they're set to DNU anyway).

@hayescr hayescr marked this pull request as ready for review June 12, 2025 23:05
@hayescr hayescr requested review from a team as code owners June 12, 2025 23:05
@hayescr
Copy link
Contributor Author
hayescr commented Jun 12, 2025

@melanieclarke thanks for taking a look already, I went ahead and made the changes you suggested, and have pulled this out of draft. Let me know if there's anything else you think should be addressed.

@melanieclarke
Copy link
Collaborator

Copy link
Collaborator
@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

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

Regtest results look as expected - a handful of new NaNs, some changes to DQ values. Thanks for the fix!

@melanieclarke melanieclarke merged commit 89d6041 into spacetelescope:main Jun 13, 2025
22 of 23 checks passed
@hayescr
Copy link
Contributor Author
hayescr commented Jun 13, 2025

Thanks for the fast review!

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.

2 participants
0