jwst
jwst copied to clipboard
Jp-3169 store aperture location in FITS header
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 https://github.com/spacetelescope/stdatamodels/pull/264 to be merged to work
Checklist for maintainers
- [x] added entry in
CHANGES.rstwithin the relevant release section - [ ] updated or added relevant tests
- [x] updated relevant documentation
- [x] added relevant milestone
- [x] added relevant label(s)
- [x] ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1266/
- [ ] Make sure the JIRA ticket is resolved properly
@hbushouse @tapastro I think this is what is needed. A change to the multspec model is needed. https://github.com/spacetelescope/stdatamodels/pull/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.
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 :warning: |
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 | ? |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Does the existing code populate the parameters that were already defined, for the extraction center in x and y?
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 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).
regression tests running: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1254/
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.
running regression tests again: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1266/
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.
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 Has there been another regtest run that fixes the hard error encountered in 1266?
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
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.
Running regression test: 1299
@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