Add miri lrs spec wcs model #393

jemorrison · 2025-02-03T22:34:05Z

Helps Resolve JP-3848

This PR replaces #382. There we too many conflicts to resolve easily after the code style changes were made.

Closes #

Corresponding jwst PR: spacetelescope/jwst#9193

This PR adds a datamodel of the reference file, specwcs, for MIRI LRS data. It can be used for fixed slit or slitless data. The reference file for fixed slit contains the V2,V3 corners of the slit. These values are not in the slitless reference file and default to None when the reference files are read in using the data models

Tasks

update or add relevant tests
update relevant docstrings and / or docs/ page
Does this PR change any API used downstream? (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)
- run jwst regression tests with this branch installed ("git+https://github.com/<fork>/stdatamodels@<branch>")

news fragment change types...

changes/<PR#>.feature.rst: new feature
changes/<PR#>.bugfix.rst: fixes an issue
changes/<PR#>.doc.rst: documentation change
changes/<PR#>.removal.rst: deprecation or removal of public API
changes/<PR#>.misc.rst: infrastructure or miscellaneous change

codecov · 2025-02-03T22:36:20Z

Codecov Report

Attention: Patch coverage is 70.83333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 78.28%. Comparing base (4e83608) to head (9c0fae9).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stdatamodels/jwst/datamodels/wcs_ref_models.py	70.83%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   78.16%   78.28%   +0.11%     
==========================================
  Files         115      110       -5     
  Lines        5144     5158      +14     
==========================================
+ Hits         4021     4038      +17     
+ Misses       1123     1120       -3

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

jemorrison · 2025-02-14T15:36:26Z

@braingram This PR is ready for another review

jemorrison · 2025-02-17T15:22:41Z

This is ready to be merged so we can get the new reference file in CRDS

braingram · 2025-02-17T15:28:34Z

Would you fix the failing style check?
https://github.com/spacetelescope/stdatamodels/actions/runs/13332329197/job/37239256929?pr=393

I think a few comments/suggestions were lost when this new PR was opened.

Would you also add a unit test (re: #382 (comment))?

Also:

Finally if this is a reference model mapped to a CRDS reference file the test_schema_against_crds file could use an update (to use the new reference model):

Is there an example file yet for this reference model? If so, would you share it so I can test this PR?

braingram · 2025-02-17T15:29:58Z

Also requesting @nden as a reviewer to follow up on comments from #382

nden · 2025-02-17T20:56:03Z

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

+---
+$schema: "http://stsci.edu/schemas/asdf/asdf-schema-1.1.0"
+id: "http://stsci.edu/schemas/jwst_datamodel/specwcs_miri_lrs.schema"
+title: MIRI LRS Spec Schema


Suggested change

title: MIRI LRS Spec Schema

title: MIRI LRS Fixed Slit Specwcs Schema

nden · 2025-02-17T21:00:53Z

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

+- $ref: keyword_exptype.schema
+- $ref: keyword_readpatt.schema
+- $ref: keyword_filter.schema
+- $ref: keyword_band.schema


Are band and readpatt really necessary?

I don't think they are needed

nden · 2025-02-17T21:02:21Z

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

+          type: number
+          title: y coord of ref position of MIRIM_SLITLESS
+          default: pixels
+          fits_keyword: IMYSLTL


Is this schema going to be used for SLITLESS mode? Do we expect a slitless mode reference file?

This schema is supposed to work for both FIX slit and slitless. The slitless data does not need the v2/v3 vertices just the FIXED slit case

nden · 2025-02-17T21:03:37Z

src/stdatamodels/jwst/datamodels/wcs_ref_models.py

+class MiriLRSSpecwcsModel(ReferenceFileModel):
+    """
+    A model for a reference file of type "specwcs" for MIRI LRS Slit.
+    The model is for the specwcs for LRS Fixed Slit and LRSSlitless


Do we expect an LRS slitless reference file?

@nden Let me clarify as understand it there is a reference file for FIXED slit and a reference file for SLITLESS. But Both of these files are read in with the same datamodel. The existing slitless reference on CRDS will be used.
A new reference file for FIXED SLIT LRS will have the v2,v3 vertices.
Do you see any problem with this. I tested reading in the new reference file for LRS FIXED slit and the existing slitless reference file using this 1 datamodel model and they are read in correctly

nden · 2025-02-17T21:05:12Z

src/stdatamodels/jwst/datamodels/wcs_ref_models.py

+    def populate_meta(self):
+        self.meta.instrument.name = "MIRI"
+        self.meta.instrument.detector = "MIRIMAGE"
+        self.meta.reftype = self.reftype


This needs more keywords to define a LRS fixed slit mode - filter?

added self.meta.instrument.filter = 'P750L'

jemorrison · 2025-02-19T16:52:40Z

@braingram I have an example reference file. How should I share it with you ?

braingram · 2025-02-19T16:57:09Z

@braingram I have an example reference file. How should I share it with you ?

Would you upload it to the linked jira ticket? The link in the description is broken, would you fix that as well?

jemorrison · 2025-02-19T17:20:05Z

@braingram
The reference file is already on the JIRI ticket: https://jira.stsci.edu/browse/JP-3848
Click on the first attached file jwst_miri_specwcs_2025jan17.fits
I just downloaded it so you should be able to get it.

jemorrison · 2025-02-19T18:02:38Z

@braingram Sorry I am not tracking.
Looking at the other specwcs reference models: NIRCAMGrismModel, NIRISSGrismModel.
These models are not defined in test_schema_against_cdrs.py
I looked at how they are used in the pipeline:
with NIRCAMGrismModel(reference_files['specwcs'])
with NIRCAMGrismModel(reference_files['specwcs'])

This is how I am using the MIRILRS one:
refmodel = MiriLRSSpecwcsModel(reference_files['specwcs'])

So do I need it in test_schema_against_crds.py ?

jemorrison · 2025-02-19T18:03:04Z

@braingram I added a unit test for this new model. I wrote this to test it was working as expected - I am not sure if it is what you want for a unit test.
Let me know if I need to change it

braingram · 2025-02-19T18:06:43Z

@braingram Sorry I am not tracking. Looking at the other specwcs reference models: NIRCAMGrismModel, NIRISSGrismModel. These models are not defined in test_schema_against_cdrs.py I looked at how they are used in the pipeline: with NIRCAMGrismModel(reference_files['specwcs']) with NIRCAMGrismModel(reference_files['specwcs'])

This is how I am using the MIRILRS one: refmodel = MiriLRSSpecwcsModel(reference_files['specwcs'])

So do I need it in test_schema_against_crds.py ?

Yeah I would say so. I believe @tapastro wrote the test originally. I think it's a bug (or lack of coverage) that NIRCAMGrismModel isn't covered but I'd say let's not extend that bug to the new model added in this PR.

jemorrison · 2025-02-19T18:17:37Z

@braingram Sorry again
This is obviously wrong because it fails. But what should it be
it seems the format is ref_type: model

"specwcs": dm.SpecwcsModel, dm.MiriLRSSpecwcsModel,
this does not work either:
"specwcs": dm.SpecwcsModel,
"specwcs": dm.MiriLRSSpecwcsModel,

jemorrison · 2025-02-19T18:26:44Z

@braingram Maybe I need to do something like cubepar_model_map does and then add it to
ref_to_multiples_dict ?
Then I can all the NIRCAM and NIRSSS ones too ?

tapastro · 2025-02-20T19:55:04Z

@jemorrison It looks like your recent updates got you most of the way there - there was a typo in the NRC_WFSS exposure type, and once that is added it uncovered a bug in the NIRCam grism model that this test was designed to catch - so I added the schema reference to fix it. We'll see if tests pass now!

src/stdatamodels/jwst/datamodels/tests/test_specwcs_mirilrs_datamodel.py

braingram

Thanks for making the test updates and pointing me to the example file.

I left a few comments about:

adding a call to validate in the unit test
clarification of the default schema values
a comment/question about the reftype in the example file causing validation failure

braingram · 2025-02-20T20:49:05Z

src/stdatamodels/jwst/datamodels/wcs_ref_models.py

+        try:
+            assert self.meta.instrument.name == "MIRI"
+            assert self.meta.instrument.detector == "MIRIMAGE"
+            assert self.meta.reftype == self.reftype


This fails with the example file which has SPECWCS instead of specwcs.

so is this really a failure of the reference file not having the correction retype ?
Is it ok to do this to account for that problem
assert self.meta.reftype == self.reftype.lower()

So I have been using the example reference file and this model to read in the data. So how does it fail for you ?

What I ran was:

>> import stdatamodels.jwst.datamodels as dm >> m = dm.MiriLRSSpecwcsModel("jwst_miri_specwcs_2025jan17.fits") >> m.validate() <ipython-input-5-6a8bfee86cf8>:1: ValidationWarning: Traceback (most recent call last): File "/Users/bgraham/projects/src/stdatamodels/src/stdatamodels/jwst/datamodels/wcs_ref_models.py", line 500, in validate assert self.meta.reftype == self.reftype ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError m.validate()

This might just be an issue with the example file. Presumably the model will be used to generate a new reference model. In that case reftype will match (since it's set in populate_meta). However if I make a blank reference model it also fails to validate:

>>> m = dm.MiriLRSSpecwcsModel() >>> m.validate() /Users/bgraham/projects/src/stdatamodels/src/stdatamodels/jwst/datamodels/reference.py:41: ValidationWarning: Model.meta is missing values for ['description', 'author', 'pedigree', 'useafter'] self.print_err(f"Model.meta is missing values for {to_fix}")

@tapastro is that expected? I'm not that familiar with how these reference models are used.

I am not sure the reference model will be used to create the reference file. It is being created in IDL.
So my test is probably not as complete as yours
from stdatamodels.jwst import datamodels
file = 'jwst_miri_specwcs_2025jan17.fits'
model = datamodels.MiriLRSSpecwcsModel(file)

that seems to work fine
When I added this line to validate MiriLRSSpecwcs function in wcs_ref_models.py assert self.meta.reftype.lower() == self.reftype
It now validates - is that cheating ?

I think that might be a crds question. @stscieisenhamer are there any special requirements for reference file reftype (are they case sensitive)?

Looking at a number of other reference files currently in CRDS it looks like those too have capitalized reftype values; e.g., APCORR, CUBEPAR, GAIN, etc.

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

jemorrison · 2025-02-20T22:41:01Z

@tapastro @braingram I am still getting an error when I run pytest test_schema_against_crds.py
I am getting an error related to --no-crds
How am I supposed to run this test. I have been just typing

pytest test_schema_against_crds.py

tapastro · 2025-02-20T23:59:39Z

@tapastro @braingram I am still getting an error when I run pytest test_schema_against_crds.py I am getting an error related to --no-crds How am I supposed to run this test. I have been just typing

pytest test_schema_against_crds.py

I had the same issue - stdatamodels has a test configuration file in the base test directory that doesn't get picked up if you run these JWST tests in their local directory. If cd to the stdatamodels base directory and run something like pytest -k selectors, it should pick up the config option.

jemorrison · 2025-02-25T16:39:25Z

@tapastro running pytest -k selectors did the trick. Now I can get test_schema_against_crds.py

@braingram I think all the changes have been made that are requested. We still need to determine about the reftype term and if it matters if it is SPECWCS or specwcs. I will ask at standup, but I can read the reference file and read in the contents so it works as expected and the retype in the reference file is SPECWCS so it seems fine to me

braingram

These suggestions update the titles to follow the convention of having the unit in square brackets at the start of the title. This convention is followed by many of the existing keywords (see the core schema for some examples) and I suggest that we follow it for new keywords.

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

braingram · 2025-02-25T19:37:01Z

Thanks. The changes pushed in 823ee64 don't match the suggestions. They contain yaml syntax errors that are causing the CI failures and the failure you're seeing. There are also still "unit" keywords left in the table datatype that I suggested to remove.

jemorrison · 2025-02-25T19:53:19Z

@braingram Ok thanks I missed the yaml errors and I misunderstood about units. I think I may finally have it.
Thanks for your help with this.

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml

braingram

Thanks for all the updates to this. The changes look good to me.

Would you run the regression tests with this PR?

Otherwise I'll leave the approval up to @nden

jemorrison · 2025-02-26T15:48:50Z

@braingram To run the regression tests on datamodels do I just select the JWST Calibration Pipeline workflow and set the git repo the spacetelescope/datamodels. Or is running a regression test on the jwst branch I have made that uses this datamodel model sufficient to test this ?
I have a first run at https://github.com/spacetelescope/RegressionTests/actions/runs/13533041283 - but I will try a second one because there are failures not related to this PR.

braingram · 2025-02-26T16:23:17Z

@braingram To run the regression tests on datamodels do I just select the JWST Calibration Pipeline workflow and set the git repo the spacetelescope/datamodels. Or is running a regression test on the jwst branch I have made that uses this datamodel model sufficient to test this ? I have a first run at https://github.com/spacetelescope/RegressionTests/actions/runs/13533041283 - but I will try a second one because there are failures not related to this PR.

I think the most useful run for this PR would be using spacetelescope/jwst (jwst main) and providing a dependency override to install the source branch for this PR git+https://github.com/jemorrison/stdatamodels.git@MIRI_LRS_Specwcs. If that passes we'd know that merging this PR wouldn't break jwst main. Would you start that run? Let me know if you have any questions or if there's anything I can do to help.

jemorrison · 2025-02-26T19:04:54Z

@braingram results of regression test:
https://github.com/spacetelescope/RegressionTests/actions/runs/13549832932

jemorrison requested a review from a team as a code owner February 3, 2025 22:34

jemorrison mentioned this pull request Feb 3, 2025

MIRI LRS specwcs datamodel #382

Closed

5 tasks

jemorrison mentioned this pull request Feb 14, 2025

JP-3848 MIRI LRS s_region and resample WCS spacetelescope/jwst#9193

Open

10 tasks

jemorrison force-pushed the MIRI_LRS_Specwcs branch from 33a6407 to f85885d Compare February 14, 2025 15:31

braingram requested a review from nden February 17, 2025 15:29

nden reviewed Feb 17, 2025

View reviewed changes

jemorrison force-pushed the MIRI_LRS_Specwcs branch from b6a1bd6 to 3b7c32d Compare February 19, 2025 18:04

jemorrison requested a review from nden February 20, 2025 16:50

braingram reviewed Feb 20, 2025

View reviewed changes

src/stdatamodels/jwst/datamodels/tests/test_specwcs_mirilrs_datamodel.py Show resolved Hide resolved

braingram reviewed Feb 20, 2025

View reviewed changes

braingram reviewed Feb 25, 2025

View reviewed changes

jemorrison force-pushed the MIRI_LRS_Specwcs branch from fd1e349 to 2306d45 Compare February 25, 2025 19:54

braingram reviewed Feb 25, 2025

View reviewed changes

src/stdatamodels/jwst/datamodels/schemas/specwcs_miri_lrs.schema.yaml Outdated Show resolved Hide resolved

braingram reviewed Feb 25, 2025

View reviewed changes

jemorrison and others added 12 commits February 26, 2025 09:02

update miri lrs spec wcs model

01e3de9

updates from review

c501111

added change log message

8837c0f

attempt to get test_schema_against_crds set up correctly

017e72f

pushing up what I have for test that is failing

5a7bcb2

fixes to pass tests

13f7bed

updates from review

2a63c89

change how validating

18bf0cf

update miri lrs schema

3743e5a

miri lrs schema update

129a90a

another try

ea05d94

update to datamodel

9c0fae9

jemorrison force-pushed the MIRI_LRS_Specwcs branch from 4dfedb4 to 9c0fae9 Compare February 26, 2025 16:03

	title: MIRI LRS Spec Schema
	title: MIRI LRS Fixed Slit Specwcs Schema

Add miri lrs spec wcs model #393

Are you sure you want to change the base?

Add miri lrs spec wcs model #393

Conversation

jemorrison commented Feb 3, 2025 • edited Loading

Tasks

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

jemorrison commented Feb 14, 2025

jemorrison commented Feb 17, 2025

braingram commented Feb 17, 2025

braingram commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jemorrison commented Feb 19, 2025

braingram commented Feb 19, 2025

jemorrison commented Feb 19, 2025

jemorrison commented Feb 19, 2025

jemorrison commented Feb 19, 2025

braingram commented Feb 19, 2025

jemorrison commented Feb 19, 2025

jemorrison commented Feb 19, 2025 • edited Loading

tapastro commented Feb 20, 2025

braingram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drlaw1558 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

jemorrison commented Feb 20, 2025

tapastro commented Feb 20, 2025

jemorrison commented Feb 25, 2025

braingram left a comment

Choose a reason for hiding this comment

braingram commented Feb 25, 2025

jemorrison commented Feb 25, 2025

braingram left a comment

Choose a reason for hiding this comment

jemorrison commented Feb 26, 2025

braingram commented Feb 26, 2025

jemorrison commented Feb 26, 2025

jemorrison commented Feb 3, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

jemorrison commented Feb 19, 2025 •

edited

Loading

drlaw1558 Feb 26, 2025 •

edited

Loading