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-3169 store aperture location in FITS header #8278

Merged
merged 10 commits into from
Mar 14, 2024

Conversation

jemorrison
Copy link
Contributor

@jemorrison jemorrison commented Feb 15, 2024

Resolves JP-3169

Closes #7773

This PR addresses adds aperture locations for valid values of xstart, xstop, ystart and ystop to the FITS header
This PR needs spacetelescope/stdatamodels#264 to be merged to work

Checklist for maintainers

@jemorrison jemorrison requested a review from a team as a code owner February 15, 2024 21:13
@jemorrison jemorrison added this to the Build 10.2 milestone Feb 15, 2024
@jemorrison
Copy link
Contributor Author

@hbushouse @tapastro
I think this is what is needed. A change to the multspec model is needed. spacetelescope/stdatamodels#264
Please commit in that PR is the keyword values I picked are ok.
I don't love them but I tried to use the existing extraction x and y center as the template.

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 53.12%. Comparing base (4cc0ac1) to head (fe8020e).
Report is 22 commits behind head on master.

Files Patch % Lines
jwst/extract_1d/extract.py 0.00% 21 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8278       +/-   ##
===========================================
- Coverage   75.15%   53.12%   -22.04%     
===========================================
  Files         470      389       -81     
  Lines       38604    38453      -151     
===========================================
- Hits        29014    20427     -8587     
- Misses       9590    18026     +8436     
Flag Coverage Δ
nightly ?

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

@tapastro
Copy link
Contributor

tapastro commented Feb 15, 2024

Does the existing code populate the parameters that were already defined, for the extraction center in x and y?

@jemorrison
Copy link
Contributor Author

The extraction center is populated for IFU data. Which is the only mode that sets the extraction_x and extraction_y.
Should other modes also fill this in ? I was thinking in the other modes there is not a defined center . I suppose we could just define it to the center of the start/stop regions.

@hbushouse
Copy link
Collaborator

@jemorrison We should probably update the extract_1d step docs to mention the fact that we're now storing the aperture limits in these new keywords. And check to see if the EXTR_X and EXTR_Y keywords for IFU extractions are already mentioned or not (and if not, add those too).

@jemorrison
Copy link
Contributor Author

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.

Needs updates to the docs.

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
jwst/extract_1d/extract.py Outdated Show resolved Hide resolved
jwst/extract_1d/extract.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator

All of our other existing keywords that record either subarray or 2-D cutout start/stop limits use values that are 1-indexed (in keeping with FITS conventions). Are these values 1-indexed or 0-indexed? I think they should be 1-indexed for consistency with all the other similar keywords.

@jemorrison
Copy link
Contributor Author

@jemorrison
Copy link
Contributor Author

The regression tests seem ok to me. I added 1 to the extraction region and I added some information on the extraction region to the docs.

@hbushouse
Copy link
Collaborator

Latest regression test results look good, except for 1 hard error:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1266/testReport/jwst.regtest/test_miri_lrs_slit_spec2/_stable_deps__test_miri_lrs_extract1d_image_ref/

Looks like an instance where xstart isn't populated at all (it's None) and hence it can't do math with the value.

@hbushouse
Copy link
Collaborator

@jemorrison Has there been another regtest run that fixes the hard error encountered in 1266?

@jemorrison
Copy link
Contributor Author

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1283/
But it is full of errors. There are so many it is hard to pull out which ones are related to this PR

@hbushouse
Copy link
Collaborator

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1283/ But it is full of errors. There are so many it is hard to pull out which ones are related to this PR

I see a lot of errors due to the removal of "product_exposure_time" from our datamodel schemas in one of the latest updates to stdatamodels. It's also been removed from use in the "resample" step code modules, so perhaps just rebasing your PR branch - to pull in those updates to resample - will fix it. There were also some NIRSpec ref file updates late last week that cause lots of expected failures in NIRSpec tests, but those truth files have all been updated now, so another run at this time should make those go away.

@jemorrison
Copy link
Contributor Author

Running regression test: 1299

@jemorrison
Copy link
Contributor Author

@tapastro @hbushouse I think this one is done. I do not really understand the last 4 tests. They fail but I don't think it is related to this PR
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1299/#showFailuresLink

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.

I agree that the last regtest run looks good.

@hbushouse hbushouse merged commit 91919b2 into spacetelescope:master Mar 14, 2024
27 of 29 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.

Record extract_1d aperture location
3 participants