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.

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

JP-1300:TSGRIM and WFSS improvements #6005

Merged
merged 2 commits into from
May 10, 2021
Merged

JP-1300:TSGRIM and WFSS improvements #6005

merged 2 commits into from
May 10, 2021

Conversation

nden
Copy link
Collaborator

@nden nden commented May 4, 2021 •
edited by hbushouse
Loading

Closes #4572

Resolves JP-1300

Description

  • Replace FITS WCS keywords in TSGRISM mode with RA/DEC_REF
  • Add Spectral system info to slits in WFSS mode

Checklist

  • Tests
  • Change log
  • Milestone
  • Label(s)

@nden nden marked this pull request as draft May 4, 2021 12:14
@nden nden added this to the Build 7.8 milestone May 4, 2021
@codecov
Copy link

codecov bot commented May 4, 2021 •
edited
Loading

Codecov Report

Merging #6005 (db98b98) into master (9219118) will decrease coverage by 0.95%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6005      +/-   ##
==========================================
- Coverage   77.76%   76.81%   -0.96%     
==========================================
  Files         399      399              
  Lines       35329    36044     +715     
==========================================
+ Hits        27473    27686     +213     
- Misses       7856     8358     +502
Flag Coverage Δ *Carryforward flag
nightly 77.76% <84.61%> (ø) Carriedforward from e09228a
unit 56.87% <95.65%> (-0.02%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
jwst/extract_2d/grisms.py 93.60% <92.59%> (+6.05%) ⬆️
jwst/assign_wcs/nircam.py 89.10% <100.00%> (ø)
jwst/pipeline/calwebb_spec2.py 77.61% <0.00%> (-18.43%) ⬇️
jwst/cube_build/ifu_cube.py 57.31% <0.00%> (-14.22%) ⬇️
jwst/cube_build/cube_build_wcs_util.py 68.78% <0.00%> (-6.22%) ⬇️
jwst/cube_build/cube_build_step.py 63.56% <0.00%> (-2.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9219118...db98b98. Read the comment docs.

@nden nden force-pushed the tsgrism-fits-wcs branch from 015d7bb to e09228a Compare May 6, 2021 19:47
@nden nden marked this pull request as ready for review May 6, 2021 19:48
@nden nden requested a review from hbushouse May 6, 2021 20:28
hbushouse
hbushouse reviewed May 7, 2021
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.

Looks good. Just a few comments and minor questions.

jwst/extract_2d/grisms.py Outdated
log.info("Spectral trace extents: (xmin: {}, ymin: {}), "
"(xmax: {}, ymax: {})".format(xmin, ymin, xmax, ymax))
log.info("Extraction limits: (xmin: {}, ymin: {}), "
"(xmax: {}, ymax: {})".format(xmin_ext, ymin, xmax, ymax))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is the same code as before, just reformatted, but why does this message use xmin_ext, while all the other items being printed are just plain ymin, xmax, ymax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The team has decided they want to cover the entire detector in the x direction, that's why it's hard coded to xmin_ext=0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that in the old code the extraction limits (line 212) arguments include xmin_ext and xmax_ext, while the new code uses xmin_ext and xmax. Should that instead be xmax_ext?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! Fixed now.

@nden nden changed the title TSGRIM and WFSS improvements JP-1330:TSGRIM and WFSS improvements May 10, 2021
@nden nden changed the title JP-1330:TSGRIM and WFSS improvements JP-1300:TSGRIM and WFSS improvements May 10, 2021
hbushouse
hbushouse approved these changes May 10, 2021
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.

LGTM

@hbushouse
Copy link
Collaborator

RTD build issue unrelated to this PR. Fixed in #6017 .

@hbushouse hbushouse merged commit 4f15658 into spacetelescope:master May 10, 2021
@stscijgbot stscijgbot mentioned this pull request May 10, 2021
Closed
@nden nden deleted the tsgrism-fits-wcs branch July 13, 2023 11:40
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

Assignees
No one assigned
Labels
Wide Field Slitless Spec (WFSS)
Projects
None yet
Milestone
Build 7.8
Development

Successfully merging this pull request may close these issues.

Refactor TSGRISM WCS transforms
2 participants
@nden @hbushouse