8000 Jp-3169 store aperture location in FITS header by jemorrison · Pull Request #8278 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Jp-3169 store aperture location in FITS header #8278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 14, 2024
Merged

Jp-3169 store aperture location in FITS header #8278

merged 10 commits into from
Mar 14, 2024

Conversation

jemorrison
Copy link
Collaborator
@jemorrison jemorrison commented Feb 15, 2024 •
edited by hbushouse
Loading

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
Collaborator 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.

@jemorrison jemorrison force-pushed the JP-3169_aper_location branch from 8ccd322 to b844100 Compare February 15, 2024 21:21
@codecov Codecov
Copy link
codecov bot commented Feb 15, 2024 •
edited
Loading

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 •
edited
Loading

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

@jemorrison
Copy link
Collaborator 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.

@jemorrison jemorrison mentioned this pull request Feb 28, 2024
Closed
@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 jemorrison force-pushed the JP-3169_aper_location branch from d46daba to 3d87881 Compare March 1, 2024 23:13
@jemorrison
Copy link
Collaborator Author

regression tests running:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1254/

hbushouse
hbushouse reviewed Mar 4, 2024
View reviewed changes
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.

@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 jemorrison force-pushed the JP-3169_aper_location branch from 3d87881 to 644415e Compare March 5, 2024 15:40
@jemorrison
Copy link
Collaborator Author

running regression tests again:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1266/

@jemorrison
Copy link
Collaborator 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.

@jemorrison jemorrison requested a review from hbushouse March 6, 2024 16:10
@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.

@jemorrison jemorrison force-pushed the JP-3169_aper_location branch from bbc34f0 to 745c077 Compare March 8, 2024 15:56
@hbushouse
Copy link
Collaborator

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

@jemorrison
Copy link
Collaborator 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 jemorrison force-pushed the JP-3169_aper_location branch from 2f0a8a1 to 8b4417b Compare March 12, 2024 13:21
@jemorrison
Copy link
Collaborator Author

Running regression test: 1299

@jemorrison jemorrison requested a review from hbushouse March 12, 2024 21:02
@jemorrison jemorrison force-pushed the JP-3169_aper_location branch from 8b4417b to fe8020e Compare March 13, 2024 16:13
@jemorrison
Copy link
Collaborator 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

hbushouse
hbushouse approved these changes Mar 14, 2024
View reviewed changes
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
@jemorrison jemorrison deleted the JP-3169_aper_location branch November 19, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbushouse hbushouse hbushouse approved these changes

@tapastro tapastro Awaiting requested review from tapastro

Assignees
No one assigned
Labels
documentation extract_1d
Projects
None yet
Milestone
Build 10.2
Development

Successfully merging this pull request may close these issues.

Record extract_1d aperture location
3 participants
@jemorrison @tapastro @hbushouse
0