Skip to content

Conversation

melanieclarke
Copy link
Collaborator

@melanieclarke melanieclarke commented Aug 27, 2025

Resolves JP-4095

Fix a missing velocity correction in the inverse transform for NIRSpec IFU. This seems to fix a round trip error of up to ~0.5 pixel. Example case: jw01251004001_03107_00001_nrs2_rate.fits, with velosys = 20 km/s.

I checked the other modes for similar errors, and found that NIRSpec MOS and FS needs the same handling. I'm not sure if we've noted similar round trip errors for these modes, but checking MOS regression test data jw01345066001_05101_00001_nrs1_rate.fits with velosys -15 km/s, it looks like it improves round trip error from ~.08 pixel to ~.001 pixel in a quick spot check on slit 35.

For the other instruments, where the velocity_correction is used, they appear to be propagating it to the inverse correctly. I did note what looks like a typo in the NIRISS SOSS handling for order 2 and 3 so I fixed it here, but I can move to another PR if it needs separate review.

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

wl2 = wl3 | velocity_corr
wl3 = wl3 | velocity_corr
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tapastro - I came across this while looking at the velocity corrections for NIRSpec WCS. It looks like a typo to me, so I fixed it here but I'm not sure what impacts it will have. Do you want me to move it to another PR for discussion with NIRISS or keep it here?

Copy link

codecov bot commented Aug 27, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.50%. Comparing base (58cc165) to head (f3eedbf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
jwst/assign_wcs/niriss.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9783   +/-   ##
=======================================
  Coverage   82.49%   82.50%           
=======================================
  Files         366      366           
  Lines       37134    37136    +2     
=======================================
+ Hits        30633    30638    +5     
+ Misses       6501     6498    -3     

☔ 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.

@melanieclarke
Copy link
Collaborator Author

melanieclarke commented Aug 27, 2025

Regression tests:
https://github.com/spacetelescope/RegressionTests/actions/runs/17276924791

It looks like these changes have a minor impact on NIRSpec MOS and IFU products for spec2. The edges of the valid IFU REGIONS change a little bit, I assume because the wavelength inversion has changed a little. For MOS and IFU, MSA flagging is impacted, which modified the DQ plane. The only SCI data affected is when nsclean is run, which I think makes sense, since the masked science regions are changing slightly.

NIRISS products do not seem to be impacted by the typo fix.

@melanieclarke melanieclarke added this to the Build 12.1 milestone Aug 27, 2025
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.

1 participant