Skip to content
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

JP-3848 MIRI LRS s_region and resample WCS #9193

Merged
merged 21 commits into from
Mar 7, 2025

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Feb 14, 2025

This PR replaces: #9104

MIRI LRS specwcs reference file PR has been merged:spacetelescope/stdatamodels#393

Resolves JP-3848

Closes #9072

This PR corrects the s_region for MIRI LRS output from assign_wcs and resample. In addition an error was found in the WCS of resampled MIRI LRS data.
In assign_wcs the s_region is determined using the V2,V3 corners of the slit. These values are provided by a new
reference file.
I added https://jira.stsci.edu/browse/JP-3910 to add a unit test for resample_spec to test the new function find_miri_lrs_sregion. I need some time to get this correct set the wcs in the unit test. My first attempts failed so I put it off into another JP ticket to get this PR in for the next delivery

Tasks

news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<PR#>.docs.rst
  • changes/<PR#>.stpipe.rst
  • changes/<PR#>.datamodels.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.set_telescope_pointing.rst
  • changes/<PR#>.pipeline.rst

stage 1

  • changes/<PR#>.group_scale.rst
  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.emicorr.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.ipc.rst
  • changes/<PR#>.firstframe.rst
  • changes/<PR#>.lastframe.rst
  • changes/<PR#>.reset.rst
  • changes/<PR#>.superbias.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.rscd.rst
  • changes/<PR#>.persistence.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.charge_migration.rst
  • changes/<PR#>.jump.rst
  • changes/<PR#>.clean_flicker_noise.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.gain_scale.rst

stage 2

  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.badpix_selfcal.rst
  • changes/<PR#>.msaflagopen.rst
  • changes/<PR#>.nsclean.rst
  • changes/<PR#>.imprint.rst
  • changes/<PR#>.background.rst
  • changes/<PR#>.extract_2d.rst
  • changes/<PR#>.master_background.rst
  • changes/<PR#>.wavecorr.rst
  • changes/<PR#>.srctype.rst
  • changes/<PR#>.straylight.rst
  • changes/<PR#>.wfss_contam.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.fringe.rst
  • changes/<PR#>.pathloss.rst
  • changes/<PR#>.barshadow.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.pixel_replace.rst
  • changes/<PR#>.resample_spec.rst
  • changes/<PR#>.residual_fringe.rst
  • changes/<PR#>.cube_build.rst
  • changes/<PR#>.extract_1d.rst
  • changes/<PR#>.resample.rst

stage 3

  • changes/<PR#>.assign_mtwcs.rst
  • changes/<PR#>.mrs_imatch.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.exp_to_source.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.tso_photometry.rst
  • changes/<PR#>.stack_refs.rst
  • changes/<PR#>.align_refs.rst
  • changes/<PR#>.klip.rst
  • changes/<PR#>.spectral_leak.rst
  • changes/<PR#>.source_catalog.rst
  • changes/<PR#>.combine_1d.rst
  • changes/<PR#>.ami.rst

other

  • changes/<PR#>.wfs_combine.rst
  • changes/<PR#>.white_light.rst
  • changes/<PR#>.cube_skymatch.rst
  • changes/<PR#>.engdb_tools.rst
  • changes/<PR#>.guider_cds.rst

Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 99.15254% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.56%. Comparing base (f479eb7) to head (8a43968).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
jwst/resample/resample_spec.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9193      +/-   ##
==========================================
+ Coverage   72.51%   72.56%   +0.05%     
==========================================
  Files         371      371              
  Lines       37092    37165      +73     
==========================================
+ Hits        26896    26970      +74     
+ Misses      10196    10195       -1     

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

@jemorrison jemorrison requested review from nden, drdavella and drlaw1558 and removed request for drdavella February 14, 2025 18:10
@drlaw1558
Copy link
Collaborator

drlaw1558 commented Feb 17, 2025

@jemorrison This is looking much better, but a few thoughts:

  • The across-slice positioning now looks good, both for S_REGIONs and for individual pixels. The along-slice is still a little weird though; from poking at this I think it's because the rotation that's being applied can change what the 'edge' pixels are slightly between cal and s2d frames (actual SOURCE positions look ok). One thing that I think we should fix though- in the individual s2d files it looks like the right edge of the resampled array is getting cut off; if I remove the ny = int(np.ceil(xstop)) and corresponding IF statement (see comment above) though then this looks better.
  • For some reason the pipeline log says again that there are NaNs in the bounding box during the assign_wcs step and that the S_REGIONS are not being updated. As a result I can't tell whether the new S_REGION code there is doing the right thing or if it's simply not changing the S_REGION from the rate files again. I'd thought that the latest pipeline was running this recalculation though, which is what kicked off much of this work in the first place?
  • I've got the new reference file ready to go as soon as the datamodels PR is merged.

@jemorrison
Copy link
Collaborator Author

@drlaw1558 are you supplying the new reference file when you run assign_wcs (spec2) in the override method ?

@drlaw1558
Copy link
Collaborator

@jemorrison Hm, I thought I had been but it looks like the new ref file wasn't getting picked up properly. With that fixed the assign_wcs indeed runs and results look good.

@jemorrison jemorrison force-pushed the JP-3848_MIRI_LRS_Second branch from 1eae471 to 1743b60 Compare February 17, 2025 15:19
@drlaw1558
Copy link
Collaborator

drlaw1558 commented Feb 17, 2025

LG2M now:
LRSwcs_new

Copy link
Collaborator

@drlaw1558 drlaw1558 left a comment

Choose a reason for hiding this comment

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

Looks good from the science side.

@melanieclarke
Copy link
Collaborator

@jemorrison - there are some unit test failures that need attention when the code structure has settled. In particular, it looks like some of the tests I wrote to make sure flux is conserved are failing with the updates to the MIRI WCS.

Copy link
Collaborator

@emolter emolter left a comment

Choose a reason for hiding this comment

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

This looks much cleaner now from an overall code design standpoint - this is exactly the flow I had in mind. I made a few additional small comments, but overall this looks great and I'm happy for it to be merged

@jemorrison jemorrison force-pushed the JP-3848_MIRI_LRS_Second branch from 120fc50 to d6ff2d5 Compare March 6, 2025 21:32
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.

Almost there!

Regression test changes look as expected, but this regression test needs a code change for the S_REGION comparison: test_miri_lrs_slit_wcs, in test_miri_lrs_slit_spec2.py.

I spotted a couple typos on reading through, noted below.

@jemorrison
Copy link
Collaborator Author

@melanieclarke I noticed the regression test failure in the test_miri_lrs_slit_wcs, in test_miri_lrs_slit_spec2.py.
But I thought it was a truth file issue. You think it is more than that ?
I have run it local to test it with an update Truth file and it runs.
But if you think there is something I am missing I can remove it and look at it when I do JP-3910

@melanieclarke
Copy link
Collaborator

But I thought it was a truth file issue. You think it is more than that ?
I have run it local to test it with an update Truth file and it runs.

Oh, I think I read it wrong - it looked like it was doing a direct assertion test that would not be okify-able from the failed run. If you were able to get it to pass with an updated truth file, then that should be fine.

@jemorrison jemorrison force-pushed the JP-3848_MIRI_LRS_Second branch from 3e1c9be to 8a43968 Compare March 7, 2025 19:20
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.

All good, thanks for the hard work on this one!

@melanieclarke melanieclarke enabled auto-merge March 7, 2025 19:34
@melanieclarke melanieclarke merged commit a5a4482 into spacetelescope:main Mar 7, 2025
25 checks passed
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.

Error in computation of MIRI LRS fixed slit S_REGION footprint
5 participants