jwst icon indicating copy to clipboard operation
jwst copied to clipboard

Jp-3169 store aperture location in FITS header

Open jemorrison opened this issue 1 year ago • 10 comments

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.rst within 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

jemorrison avatar Feb 15 '24 21:02 jemorrison

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

jemorrison avatar Feb 15 '24 21:02 jemorrison

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.

codecov[bot] avatar Feb 15 '24 21:02 codecov[bot]

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

tapastro avatar Feb 15 '24 22:02 tapastro

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 avatar Feb 16 '24 17:02 jemorrison

@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).

hbushouse avatar Mar 01 '24 13:03 hbushouse

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

jemorrison avatar Mar 01 '24 23:03 jemorrison

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.

hbushouse avatar Mar 04 '24 19:03 hbushouse

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

jemorrison avatar Mar 05 '24 16:03 jemorrison

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 avatar Mar 06 '24 16:03 jemorrison

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 avatar Mar 08 '24 13:03 hbushouse

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

hbushouse avatar Mar 11 '24 12:03 hbushouse

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

jemorrison avatar Mar 11 '24 12:03 jemorrison

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.

hbushouse avatar Mar 11 '24 12:03 hbushouse

Running regression test: 1299

jemorrison avatar Mar 12 '24 13:03 jemorrison

@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

jemorrison avatar Mar 13 '24 16:03 jemorrison