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-3595: handling for NIRCam GRISM Time Series Pointing offset SR #8449

Merged
merged 5 commits into from May 8, 2024

Conversation

emolter
Copy link
Contributor

@emolter emolter commented Apr 29, 2024

Resolves JP-3595

Closes #8420

This PR adds support for extraction of spectra in NIRCam GRISM TSO mode when an Offset Special Requirement is specified, which manifests as a nonzero dither XOFFSET. This is handled in extract_2d. A unit test was added covering the function that computes this offset.

One unit test of assign_wcs was also modified so that its xfail could be removed.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@emolter
Copy link
Contributor Author

emolter commented Apr 29, 2024

Regression test run started here

Per our discussion at stand-up today, I found a way to factor this that I'm happy with, which was to modify primarily extract_2d, pulling the requisite transforms from the wcs object. There is no longer a need to "remember" the pixel value of the offset between steps, nor compute it twice. The only slightly strange thing is that now wcsinfo.siaf_xref_sci is modified in extract_2d, as it must be offset to allow extract_1d to work right.

I'd like input on how to handle regtests for this change. We could either add an entirely new rate file as input and extract_2d file as output, or change the current nircam TSO test data to be a dataset with an offset. Or, we could write no new regtest at all - the lines are technically covered by the current regtest, it just finds an offset near zero.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.56%. Comparing base (6580914) to head (d38a4dc).
Report is 10 commits behind head on master.

❗ Current head d38a4dc differs from pull request most recent head e9ba9de. Consider uploading reports for the commit e9ba9de to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8449      +/-   ##
==========================================
+ Coverage   56.38%   56.56%   +0.17%     
==========================================
  Files         387      387              
  Lines       38716    38797      +81     
==========================================
+ Hits        21830    21944     +114     
+ Misses      16886    16853      -33     

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

@emolter
Copy link
Contributor Author

emolter commented Apr 30, 2024

Another regtest run for just the tsgrism stuff, including one new regtest, is here

@emolter emolter marked this pull request as ready for review April 30, 2024 15:13
@emolter emolter requested a review from a team as a code owner April 30, 2024 15:13
@hbushouse hbushouse added this to the Build 11.0 milestone May 7, 2024
Copy link
Collaborator

@hbushouse hbushouse 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 overall. Just a few minor questions and comments.

jwst/assign_wcs/nircam.py Show resolved Hide resolved
jwst/extract_2d/grisms.py Outdated Show resolved Hide resolved
jwst/extract_2d/grisms.py Outdated Show resolved Hide resolved
jwst/extract_2d/grisms.py Outdated Show resolved Hide resolved
jwst/extract_2d/grisms.py Outdated Show resolved Hide resolved
jwst/extract_2d/tests/test_grisms.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

Another regtest run for just the tsgrism stuff, including one new regtest, is here

Note for the record that the regtest run was completely clean (no failures).

@hbushouse
Copy link
Collaborator

I wonder if there's some place in RTD that this procedure can be mentioned. Is there any place where we already mention that for TSGRISM observations we assume the target is located at XREF_SCI/YREF_SCI? If so, that could be updated to explain how we now also check for offsets.

@emolter
Copy link
Contributor Author

emolter commented May 7, 2024

I'm checking the docs now

@emolter
Copy link
Contributor Author

emolter commented May 7, 2024

The pixel values where the source is centered are discussed on the readthedocs page of extract_2d, which I will update, as well as on this page, which isn't exactly readthedocs, but nevertheless it might be a good place to state that in rare cases one might wish to offset the center pixel in X and that this is supported.

Copy link
Collaborator

@hbushouse hbushouse 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 to me.

@hbushouse hbushouse merged commit 9e857a5 into spacetelescope:master May 8, 2024
23 of 24 checks passed
@emolter emolter deleted the JP-3595 branch May 8, 2024 21:01
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.

None yet

2 participants