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

PSF data model #336

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

Conversation

jemorrison
Copy link
Contributor

@jemorrison jemorrison commented Oct 16, 2024

Supports JP-251

This PR adds a PSF reference data model to support PSF-based extraction for MIRS LRS FIXED slit data in the JWST pipeline.

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

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.60%. Comparing base (1e16207) to head (619da9d).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   67.52%   67.60%   +0.07%     
==========================================
  Files         114      115       +1     
  Lines        5916     5927      +11     
==========================================
+ Hits         3995     4007      +12     
+ Misses       1921     1920       -1     

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

@jemorrison
Copy link
Contributor Author

@spacetelescope/stdatamodels-maintainers ready for review

@jemorrison
Copy link
Contributor Author

@braingram @emolter I could use a review of this code to get it into the pipeline to be used with the new reference file

@braingram
Copy link
Collaborator

Is this in support of spacetelescope/jwst#8967?
If so, would you help me find where this new model is used in that PR?
If not, is there an associated JWST PR?

Also, is there an example file?

@melanieclarke
Copy link

Is this in support of spacetelescope/jwst#8967? If so, would you help me find where this new model is used in that PR? If not, is there an associated JWST PR?

Also, is there an example file?

That is the right PR for this change. The new model is used here:
https://github.com/spacetelescope/jwst/blob/521fa4c0c6cb6b7cfcf7a07700d812accc67e60a/jwst/extract_1d/psf_profile.py#L92

Example file is on CRDS test:
https://jwst-crds-test.stsci.edu/browse/jwst_miri_psf_0001.fits

The PR for JP-251 has not yet been updated to match this format for the file though -- it currently expects an earlier version of the file, with a 1D wavelength array.

@braingram
Copy link
Collaborator

Is this in support of spacetelescope/jwst#8967? If so, would you help me find where this new model is used in that PR? If not, is there an associated JWST PR?
Also, is there an example file?

That is the right PR for this change. The new model is used here: https://github.com/spacetelescope/jwst/blob/521fa4c0c6cb6b7cfcf7a07700d812accc67e60a/jwst/extract_1d/psf_profile.py#L92

Example file is on CRDS test: https://jwst-crds-test.stsci.edu/browse/jwst_miri_psf_0001.fits

The PR for JP-251 has not yet been updated to match this format for the file though -- it currently expects an earlier version of the file, with a 1D wavelength array.

Thanks!

@braingram
Copy link
Collaborator

It looks like the reference file is only on test at the moment. I suspect we will want to update test_schema_against_crds.py to include the new reference file datamodel/reftype (before this line if the reftype is 'psf'):


If we make that update as part of this PR will it break this test? Also is there a way to run that test against jwst-test to confirm the update/model is working?

@jemorrison
Copy link
Contributor Author

@melanieclarke @braingram I would think that if the reference file is only in crds-test that if we update the test test_schema_against_crds.py it would break the system.
Can we update that test once this reference file is in the operation crds. This is not needed for the upcoming build so I would not want to break anything getting ready for that build.

@braingram
Copy link
Collaborator

@melanieclarke @braingram I would think that if the reference file is only in crds-test that if we update the test test_schema_against_crds.py it would break the system. Can we update that test once this reference file is in the operation crds. This is not needed for the upcoming build so I would not want to break anything getting ready for that build.

My hunch is that if we update the test now to know that psf reftypes are opened with MiriLrsPsfModel then that mapping won't be used for the current crds (since no imap has a psf rmap). When test becomes ops and there is a psf rmap the test will (ideally) continue to pass (and I was hoping we could have some assurance by testing against test).

If we don't update the test when test gets mapped to ops the test_schema_against_crds.py will find a psf rmap and fail since it doesn't know what model to map it to.

I will try to look at this today or tomorrow.

@jemorrison
Copy link
Contributor Author

@melanieclarke Change the wavelength array back to 1 D

@braingram
Copy link
Collaborator

I will try to look at this today or tomorrow.

I tested this PR with jwst-crds-test and it fails due to the missing reftype mapping in the test_schema_against_crds.py. Adding the following allows it to pass with jwst-crds-test and jwst-crds.

+    'psf': dm.MiriLrsPsfModel,
     'psfmask': dm.PsfMaskModel,

@jemorrison would you update the above mentioned test with the change so that test won't start failing when ops is updated?

@@ -0,0 +1 @@
added MIRI LTS FIXED SLIT PSF reference model
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
added MIRI LTS FIXED SLIT PSF reference model
added MIRI LRS FIXED SLIT PSF reference model

@braingram braingram mentioned this pull request Dec 23, 2024
5 tasks
@braingram
Copy link
Collaborator

I will try to look at this today or tomorrow.

I tested this PR with jwst-crds-test and it fails due to the missing reftype mapping in the test_schema_against_crds.py. Adding the following allows it to pass with jwst-crds-test and jwst-crds.

+    'psf': dm.MiriLrsPsfModel,
     'psfmask': dm.PsfMaskModel,

@jemorrison would you update the above mentioned test with the change so that test won't start failing when ops is updated?

The psf reftype is now on ops. I added an ignore for this reftype in #371 that we can remove in this PR (along with the above mentioned changes).

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Now that #371 is merged (with the "skip" for the psf reftype) when you update this PR would you remove the "skip" and update the test as mentioned in: #336 (comment)

Thanks!

@melanieclarke
Copy link

I'm testing out what would need to be done to extend this work to NIRSpec, and I'm wondering if there is anything MIRI LRS specific about this model. Would it make sense to name it something more general so we can use the same model for other modes? Like maybe call it SlitPSFModel, include a dispersion axis, and rename the center_col keyword to cross_dispersion_center or something like that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants