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

Update MultiSpec schema to provide keywords for recording extraction characteristics #264

Merged
merged 4 commits into from
Feb 29, 2024

Conversation

jemorrison
Copy link
Contributor

@jemorrison jemorrison commented Feb 15, 2024

Partially resolves JP-3169

Is needed to close spacetelescope/jwst#7773

This PR updates the MultiSpec model to include the xstart, xstop, ystart, ystop values (if they are defined) so they can be recorded in the FITS header

The extraction x center and y center were already defined with keyword values of EXTR_X, EXTR_Y.
Since the extraction locations are related to these values I picked
EXTRXSTR: x start of extraction region
EXTRXSTP: x stop of extraction region
EXTRYSTR: y start of extraction region
EXTRYSTP: y stop of extraction region

But these value can be changed if a better set can be found

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@jemorrison jemorrison requested a review from a team as a code owner February 15, 2024 20:05
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.87%. Comparing base (4d7c3a6) to head (bb886a7).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   64.84%   64.87%   +0.03%     
==========================================
  Files         103      104       +1     
  Lines        5694     5699       +5     
==========================================
+ Hits         3692     3697       +5     
  Misses       2002     2002              

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

@jemorrison
Copy link
Contributor Author

@hbushouse @tapastro I could not add you to the list of reviewers. Are the names of the FITS Keywords for these parameters ok to you. This PR will need to be merged before the extract 1d code updates will be able to use them

@hbushouse hbushouse changed the title Update multi spec schema to report extraction location in FITs Headers of x1d products Update MultiSpec schema to provide keywords for recording extraction characteristics Feb 16, 2024
@hbushouse
Copy link
Contributor

We already have a couple of similar sets of keywords that give region limits:

SUBSTRT1, SUBSTRT2, SUBSIZE1, SUBSIZE2 - which record the location and size of subarrays relative to the full detector
SLTSTRT1, SLTSTRT2, SLTSIZE1, SLTSIZE2 - which record the location and size of 2-D cutouts performed in extract_2d

These work a bit differently, in that instead of including explicit start/stop values, they only specify the start location and then record the size.

So if we want keywords that record start/stop values, we can't just duplicate these existing sets by simply changing the prefix (i.e. use "EXTR" instead of "SUB" and "SLT"). But I'm wondering if we should at least conform to the same use of axis numbers in the keywords, instead of using "x" and "y"? Unfortunately we already have a precedent set with the "EXTR_X" and "EXTR_Y" keywords that are used to record the center of the circular apertures used to make 1-D extractions from IFU cubes. So ... not sure what the best naming convention should be for these new keywords.

Not a big fan of the fact that "EXTRXSTR" and "EXTRXSTP" are so similar looking until you see the difference in the very last letter. But I honestly don't have many better suggestions. So I guess, after all of this, I'm OK with the proposed keyword names.

Copy link
Contributor

@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 a change log entry

@hbushouse hbushouse force-pushed the extract_1d_aper_location branch from f283136 to 378aa9e Compare February 29, 2024 01:06
@hbushouse hbushouse merged commit 80d90c3 into spacetelescope:main Feb 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record extract_1d aperture location
2 participants