Skip to content

JP-3944: Add chromcorr handling to assign_wcs for nirspec#10556

Open
emolter wants to merge 25 commits into
spacetelescope:mainfrom
emolter:JP-3944
Open

JP-3944: Add chromcorr handling to assign_wcs for nirspec#10556
emolter wants to merge 25 commits into
spacetelescope:mainfrom
emolter:JP-3944

Conversation

@emolter

@emolter emolter commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Resolves JP-3944

See companion stdatamodels PR here: spacetelescope/stdatamodels#743

This PR adds a chromaticity correction to the NIRSpec IFU assign_wcs pipeline. The correction is delivered via a new "chromcorr" reference file and associated ChromCorrModel.

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)
      • if your change breaks step-level or public API (as defined in the docs), also add a changes/<PR#>.breaking.rst news fragment
    • 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

Co-authored-by: Christian Hayes <chayes@stsci.edu>
@emolter emolter changed the title Add chromcorr handling to assign_wcs for nirspec JP-3944: Add chromcorr handling to assign_wcs for nirspec May 18, 2026
@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.86%. Comparing base (9136ddc) to head (31a52c8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10556   +/-   ##
=======================================
  Coverage   86.86%   86.86%           
=======================================
  Files         375      375           
  Lines       40346    40356   +10     
=======================================
+ Hits        35047    35056    +9     
- Misses       5299     5300    +1     

☔ View full report in Codecov by Harness.
📢 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.

@emolter

emolter commented May 18, 2026

Copy link
Copy Markdown
Collaborator Author

regtests pointed to stdatamodels PR branch: https://github.com/spacetelescope/RegressionTests/actions/runs/26053406592

@melanieclarke melanieclarke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks reasonable to me. I think the new "oteip_chromcorr" frame needs to be noted in a few more places. I marked a couple, but it might be a good idea to look around for more places where the specific frames for NIRSpec are mentioned.

Do you know if any of the unit tests for the NIRSpec WCS will need updating? I'm not sure how to test that before the files are available.

It would also be helpful to get a review from one of the WCS experts if possible.

Comment thread docs/jwst/assign_wcs/exp_types.rst Outdated
Comment thread jwst/assign_wcs/nirspec.py
Comment thread jwst/assign_wcs/nirspec.py
@emolter

emolter commented May 27, 2026

Copy link
Copy Markdown
Collaborator Author

@melanieclarke who are considered the "wcs experts" these days?

@melanieclarke

Copy link
Copy Markdown
Collaborator

@melanieclarke who are considered the "wcs experts" these days?

I think @mcara and @WilliamJamieson.

@emolter

emolter commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

@melanieclarke recent commits have added mentions of the oteip_chromcorr frame in (hopefully) all relevant places

@emolter

emolter commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

https://github.com/spacetelescope/RegressionTests/actions/runs/27158036984 regression tests pointing to crds-test.

All the regression tests on real data look as expected, with most failures coming from a new R_CHRCOR keyword and a few small numerical diffs in the NIRSpec IFU tests.

There are a few failures in unit tests that need investigating, which all show no attribute "inverse". I assume these are due to some kind of incomplete mocked wcs. I'll look into this tomorrow.

edit: thanks to @hayescr for helping me figure this out, this was due to the CRDS selection rules requiring a date > 2022-01-01, i.e., post-launch. Some of our mock data had dates prior to that. I just updated the dates in 183184f
and restarted the regression tests

Comment thread docs/jwst/assign_wcs/exp_types.rst
Comment thread jwst/assign_wcs/nirspec.py
Comment thread jwst/regtest/test_nirspec_ifu_spec2.py Outdated

@emolter emolter Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

tolerances in the IFU tests in file needed to be relaxed a lot to get tests to pass. It makes sense that the WCS solution has changed due to the new files, plus changing the date from pre-launch to 2026, so I don't think this is a particular concern. However it may be useful in the future to consider how to restructure these tests to avoid testing against these now out-of-date truth files.

@emolter

emolter commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/27432609830

There are several hundred failures because all products that go through assign_wcs now get the keyword R_CHRCOR set to "N/A".

Additionally, for some NIRSpec IFU tests, there are small differences in the S_REGION keyword.
There are numerical diffs in two tests, test_nirspec_ifu_spec2_trace_model[s3d]: jwst.regtest.test_nirspec_ifu_trace_model and test_nirspec_ifu_spec2_trace_model[x1d]: jwst.regtest.test_nirspec_ifu_trace_model. Both are related to NIRSpec IFU and I'm happy to assume they're both expected unless told otherwise.

Comment thread pyproject.toml

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CRDS bump required to pick up spacetelescope/crds@3a6bea3

Apparently this is the first time we've had a new reftype in an ops context that's not matched to a release .

@emolter

emolter commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

I think the oldestdeps failures now are unrelated:

E                   asdf.exceptions.AsdfPackageVersionWarning: File 'file:///tmp/data/crds_cache/references/jwst/nirspec/jwst_nirspec_pars-cleanflickernoisestep_0001.asdf' was created with extension URI 'asdf://asdf-format.org/core/extensions/core-1.6.0' (from package asdf==5.1.0), but older package (asdf==4.2.0) is installed.
/home/runner/work/jwst/jwst/.tox/py3-oldestdeps-xdist-cov/lib/python3.11/site-packages/asdf/_asdf.py:307: AsdfPackageVersionWarning

They don't have to do with chromcorr at all.

@emolter emolter marked this pull request as ready for review June 12, 2026 19:22
@emolter emolter requested a review from a team June 12, 2026 19:22
@emolter emolter requested review from hayescr and melanieclarke June 12, 2026 19:22
@melanieclarke

Copy link
Copy Markdown
Collaborator

I think the oldestdeps failures now are unrelated:
...
They don't have to do with chromcorr at all.

We should probably bump asdf anyway, or else NIRSpec users with older installed asdf versions will see this warning.

@melanieclarke

Copy link
Copy Markdown
Collaborator

Looks like there's a slow unit test for pixel replace that's failing in the regtests:
test_pixel_replace_nirspec_ifu[fit_profile-nirspec_ifu]: jwst.pixel_replace.tests.test_pixel_replace

Spot checking the rest looks like expected differences.

@emolter

emolter commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

to clarify you want me to put that ASDF bump into this PR?

I'll have a look at the slow test, thanks for catching it

@emolter

emolter commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

@melanieclarke pix replace test fixed by 31a52c8

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.

5 participants