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 Update S_REGION for MIRI LRS fixed slit data #9104

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jemorrison
Copy link
Collaborator

@jemorrison jemorrison commented Jan 28, 2025

Resolves JP-3848

This PR needs a new datamodel that defines MIRI LRS specwcs reference files. The stdatamodels for this change is
spacetelescope/stdatamodels#383. The datamodel PR must be merged before this PR can be
merged.

THIS PR IS NOT READY TO BE MERGED. We still need to make changes to the resample code when it updates the S_REGION for MIRI LRS data.

This PR is not correct for updating the s-region for spec 3 MIRI LRS data. Still testing code.

Closes #

This PR correctly updates the s_region set by assign_wcs for MIRI LRS fixed slit data. The change requires using the slit corners in V2,V3 space which were added to the MIRI LRS specwcs reference file.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (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 below for change types)
    • 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
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#>.fits_generator.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

@jemorrison jemorrison requested a review from a team as a code owner January 28, 2025 20:38
@jemorrison jemorrison removed the MIRI label Jan 28, 2025
@jemorrison jemorrison added this to the Build 11.3 milestone Jan 28, 2025
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 1.06383% with 93 lines in your changes missing coverage. Please review.

Project coverage is 62.70%. Comparing base (cbba455) to head (20168fc).
Report is 113 commits behind head on main.

Files with missing lines Patch % Lines
jwst/resample/resample_utils.py 0.00% 33 Missing ⚠️
jwst/assign_wcs/util.py 4.34% 22 Missing ⚠️
jwst/assign_wcs/miri.py 0.00% 21 Missing ⚠️
jwst/resample/resample_spec_step.py 0.00% 11 Missing ⚠️
jwst/assign_wcs/assign_wcs.py 0.00% 5 Missing ⚠️
jwst/resample/resample_spec.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9104       +/-   ##
===========================================
- Coverage   78.23%   62.70%   -15.53%     
===========================================
  Files         505      507        +2     
  Lines       46249    47349     +1100     
===========================================
- Hits        36183    29691     -6492     
- Misses      10066    17658     +7592     
Flag Coverage Δ *Carryforward flag
nightly 77.77% <ø> (-0.02%) ⬇️ Carriedforward from ae0157d

*This pull request uses carry forward flags. Click here to find out more.

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

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

  • The code as written does not allow using the old file. Is this desirable to make the transition easier?

  • Line 317 was added to account for the fact that the Tabular model defines its own range for inputs. If the bounding_box is larger then s_region will be filled with NaNs because this is the fill_value for the Tabular model. It's unlikely the slit corners are outside the range of the Tabular model so those lines should probably be removed.

  • Also this needs some kind of test. It may be an addition to an existing test to check the s_region has a reasonable value.

@@ -807,6 +807,37 @@ def update_s_region_imaging(model):
model.meta.wcsinfo.s_region = s_region


def update_s_region_lrs(model, reference_files):
"""
Update the ``S_REGION`` keyword using V2,V3 of the slit corners from reference file`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Update the ``S_REGION`` keyword using V2,V3 of the slit corners from reference file`.
Update the ``S_REGION`` keyword using V2,V3 of the slit corners from reference file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nden I need a clarification on not needing dxmodel (line 317 any more)
Does that mean that we no longer need to determine bb_sub in line 311. So lines 301 to 311 can me cut:
These lines:

Fit for X shift as a function of Y

# Spline fit with enforced smoothness
spl = UnivariateSpline(ycen_subarray[::-1], xcen_subarray[::-1] - zero_point[0], s=0.002)
# Evaluate the fit at the y reference points
xshiftref = spl(ycen_subarray)
# This function will give slit dX as a function of Y subarray pixel value
dxmodel = models.Tabular1D(lookup_table=xshiftref, points=ycen_subarray, name='xshiftref',
                             bounds_error=False, fill_value=np.nan)

if input_model.meta.exposure.type.lower() == 'mir_lrs-fixedslit':
    bb_sub = (bb_sub[0], (dxmodel.points[0].min(), dxmodel.points[0].max()))

v2 = [v2vert1, v2vert2, v2vert3, v2vert4]
v3 = [v3vert1, v3vert2, v3vert3, v3vert4]

lam = None # wavelength does not matter for s region
Copy link
Collaborator

Choose a reason for hiding this comment

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

Transforms usually require numerical inputs. Although it might not crash in this specific case it makes sense to set it to a number.

zero_point = ref[0].header['imxsltl'] - 1, ref[0].header['imysltl'] - 1
# Transform to slitless subarray from full array
zero_point = subarray2full.inverse(zero_point[0], zero_point[1])
refmodel = MIRILrsModel(reference_files['specwcs'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

refmodel will need to be closed at the end

zero_point = ref[0].header['imxsltl'] - 1, ref[0].header['imysltl'] - 1
# Transform to slitless subarray from full array
zero_point = subarray2full.inverse(zero_point[0], zero_point[1])
refmodel = MIRILrsModel(reference_files['specwcs'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

close the file at the end

Update the ``S_REGION`` keyword using V2,V3 of the slit corners from reference file`.
"""

refmodel = MIRILrsModel(reference_files['specwcs'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

close the file

@jemorrison
Copy link
Collaborator Author

@nden If I remove following in miri.py

dxmodel = models.Tabular1D(lookup_table=xshiftref, points=ycen_subarray, name='xshiftref',

#                             bounds_error=False, fill_value=np.nan)

Then later on in the code dxmodel is used. How should xmodel be defined:

What is the effective slit X as a function of subarray x,y?

xmodel = models.Mapping([0], n_inputs=2) - (models.Mapping([1], n_inputs=2) | dxmodel)

@jemorrison
Copy link
Collaborator Author

@nden
You were concerned:
"As the code as written does not allow using the old file. Is this desirable to make the transition easier?'
As it is written if it uses the old reference file the v2 v3 values are set to NaN and we get the message:
025-01-31 14:43:49,200 - stpipe.AssignWcsStep - INFO - There are NaNs in s_region, S_REGION not updated.

So the s_region is not updated and it is just what set telescope pointing set which I think is more desirable that the what the
old code did and updated the s_region to very incorrect values.
Is this acceptable to you ? I have also added an INFO message that the V2,V3 corners in the reference file are missing and signaling that no updated s_region will result.

@jemorrison
Copy link
Collaborator Author

@nden
I am working on a test. I would like to set up an LRS data model using a hard coded set of wcs values and s_region.
I can also set up the lrspec wcs reference file.
Then call the update_s_region_lrs in util.py - that needs the v2v3 transform to world and test that new s_region similar to the original s_region (diff less than some tolerance)
I am just not sure how to set up the wcs of the lrs data. When I look at the wcs of a cal lrs miri file - well it contain A LOT of info. Could you give me a suggestion on how to get started ?

@github-actions github-actions bot added the MIRI label Jan 31, 2025
@nden
Copy link
Collaborator

nden commented Feb 3, 2025

@jemorrison regarding setting up the WCS of a LRS cal file - could you create in memory ImageModel with the appropriate configuration and call AssignWcs on it. Alternativelly add a check for s_region in one of the existing regression tests. May be this one will work.

I agree with the decision about the old file.

Also, I suggest you remove the lines below, not dxmodel. These reset the bounding_box and will have an effect on computing s_region.

if input_model.meta.exposure.type.lower() == 'mir_lrs-fixedslit':
    bb_sub = (bb_sub[0], (dxmodel.points[0].min(), dxmodel.points[0].max()))

@jemorrison
Copy link
Collaborator Author

@nden What I have written in resample_utils.py combine_s_regions_lrs is not correct.
This code will depend on the PA. I did not want to change the wcs for the lrs in resample_spec since the data is being combined correctly - all that is wrong is the final s_region values in the header.
Maybe I can remove the PA then find the combined_s_region using the code I have (or something similar) and then apply the PA to the footprint values.
Suggestions ?

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