Skip to content

Conversation

@emolter
Copy link
Collaborator

@emolter emolter commented Nov 10, 2025

Resolves JP-3307

Closes #7728

This PR adds a new step to the spec2 pipeline for MIRI LRS fixed-slit and slitless observations. See ticket and docstring updates for details.

This PR also modifies the association rules to add MIR_TACONFIRM type exposures to MIRI LRS slit and slitless spec2 associations.

Requires spacetelescope/stdatamodels#620

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

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

❌ Patch coverage is 97.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.00%. Comparing base (1193035) to head (a6c2ced).
⚠️ Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
jwst/resample/resample_spec_step.py 45.45% 6 Missing ⚠️
jwst/pipeline/calwebb_spec2.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10011      +/-   ##
==========================================
+ Coverage   85.72%   86.00%   +0.27%     
==========================================
  Files         367      368       +1     
  Lines       38235    38453     +218     
==========================================
+ Hits        32776    33070     +294     
+ Misses       5459     5383      -76     

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

@emolter
Copy link
Collaborator Author

emolter commented Nov 11, 2025

@emolter emolter added this to the Build 12.3 milestone Nov 11, 2025
@melanieclarke
Copy link
Collaborator

@melanieclarke f7079b3 fixes a major bug in the last try and appears to give the expected position for all the real datasets I have. I still need to update the unit test mock data to have a more realistic WCS, so the unit tests are failing for now.

Great, thanks! I can check again, but probably not until Tuesday.

What's the best way to get a fiducial wavelength for a SlitModel? For now I put an (incomplete) lookup table in-line in the code, but that's obviously not ideal.

For extraction purposes, I have used this function to get the effective middle of the slit:

def middle_from_wcs(wcs, bounding_box, dispaxis):

@emolter
Copy link
Collaborator Author

emolter commented Dec 29, 2025

@melanieclarke I think this is ready for another review. Ian messaged me on Slack saying things were looking good as far as he could tell. I am not sure what test data he ran through. I checked some LRS nod data and it looks ok to me although I'm no expert.

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.

Testing looks good now, for both nodded slit and slitless cases. Thanks for the fixes!

Code and test suite also look good to me. I have a few more minor suggestions for documentation and logging.

Also, I think we should add a regression test to exercise the step on real data.

@emolter
Copy link
Collaborator Author

emolter commented Dec 29, 2025

@melanieclarke all your individual comments should be addressed by 4c1fea4

I'll work on the regression test.

@melanieclarke
Copy link
Collaborator

melanieclarke commented Dec 29, 2025

@melanieclarke all your individual comments should be addressed by 4c1fea4

Thanks, the log looks much cleaner now.

@emolter
Copy link
Collaborator Author

emolter commented Dec 30, 2025

regtests including new one: https://github.com/spacetelescope/RegressionTests/actions/runs/20600657582

Here is a list of regression test differences. There are a lot, but I think all are expected. We will need to run new regtests once we get the stdatamodels PR in.

  • 4 diffs due to asn changes, starting with test_std. All of these show "Right is a subset of left", meaning the new asn is the same as the old one but with an extra member. All of those extra members are labeled "target_acquisition"
  • 10 diffs in test_nirspec_fs_*, all due to extra TA_XPOS and TA_YPOS
  • 23 diffs in test_nirspec_mos* due to same
  • 8 diffs due to same in other tests (test_nirspec_brightobj, test_nircam_tsgrismx4, test_tso3_x1dints, test_nirspec_lamp, test_nirspec_bots)
  • 12 diffs starting with test_spec2 due to new TA_CNTR status keyword being set to skipped
  • 1 diff due to same in test_nirspec_imprint
  • 11 diffs in test_miri_mrs_* due to same
  • 5 diffs in test_miri_lrs_slitless_tso_spec2, mainly due to change from CubeModel to SlitModel: extensions have been reordered, with int_times now after variance arrays instead of before - the only data "differences" are due to those reorderings. There are also several new metadata fields included.
  • 3 diffs in test_miri_lrs_slitless_tso3 due to same
  • 3 diffs in new added regtest test_miri_lrs_slitless_spec2_targ_centroid because I forgot to create the truth data on the new stdatamodels branch. The differences show extra TA_CNTR status set to complete, and extra TA_XPOS and TA_YPOS. So these are benign and can be okified.
  • 15 diffs in test_miri_lrs_nod* all due to change from ImageModel to SlitModel. New metadata but no data differences.
  • 9 diffs in test_miri_lrs_slit_spec2, which I modified to include targ_centroid. Some are because I forgot to create the truth data on the new stdatamodels branch. The differences show extra TA_CNTR status set to complete, and extra TA_XPOS and TA_YPOS. So these are benign and can be okified. Some others are due to the targ_centroid step getting run, namely different values of SRCXPOS and SRCYPOS keywords
  • 5 diffs in test_miri_lrs_slit_spec2_optimal due to change from ImageModel to SlitModel, no data changes.

@melanieclarke
Copy link
Collaborator

Okay, that all sounds reasonable to me, thanks for spelling it out! I'm ready to approve and will let you handle the merge and okify. Let me know if you need anything.

@emolter
Copy link
Collaborator Author

emolter commented Dec 31, 2025

run for okify. will merge when complete: https://github.com/spacetelescope/RegressionTests/actions/runs/20628286454

@emolter emolter merged commit 8147b65 into spacetelescope:main Jan 1, 2026
33 of 38 checks passed
@emolter emolter deleted the JP-3307 branch January 1, 2026 00:11
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.

MIRI LRS - Determine source position from TA verification image

3 participants