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-1829: Fix resample_spec output size from input images crossing RA=0 #5929

Merged
merged 5 commits into from
Apr 12, 2021
Merged

JP-1829: Fix resample_spec output size from input images crossing RA=0 #5929

merged 5 commits into from
Apr 12, 2021

Conversation

jdavies-st
Copy link
Collaborator

@jdavies-st jdavies-st commented Apr 6, 2021 •
edited
Loading

Closes #5564
Resolves JP-1829

Description

This PR fixes the case where the WCS of the input images spans RA=0.

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

@jdavies-st jdavies-st added this to the Build 7.8 milestone Apr 6, 2021
@codecov
Copy link

codecov bot commented Apr 6, 2021 •
edited
Loading

Codecov Report

Merging #5929 (c4c5fe0) into master (cd5fcd4) will increase coverage by 0.06%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5929      +/-   ##
==========================================
+ Coverage   77.91%   77.97%   +0.06%     
==========================================
  Files         403      403              
  Lines       35392    35573     +181     
==========================================
+ Hits        27575    27739     +164     
- Misses       7817     7834      +17
Flag Coverage Δ *Carryforward flag
nightly 77.91% <100.00%> (ø) Carriedforward from cd5fcd4
unit 56.04% <81.25%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
jwst/resample/resample_spec.py 94.01% <88.23%> (-2.44%) ⬇️
jwst/resample/resample_spec_step.py 93.91% <100.00%> (+3.39%) ⬆️
jwst/associations/lib/rules_level2b.py 93.84% <0.00%> (-2.87%) ⬇️
jwst/associations/lib/rules_level3.py 98.75% <0.00%> (-0.49%) ⬇️

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 cd5fcd4...c4c5fe0. Read the comment docs.

@jdavies-st jdavies-st requested review from jemorrison and nden April 6, 2021 02:37
@jdavies-st jdavies-st changed the title JP-1829: Fix resample_spec were range of input images crosses RA=0 JP-1829: Fix resample_spec output size from input images crossing RA=0 Apr 6, 2021
@jdavies-st jdavies-st force-pushed the JP-1829-resample-spec-zero-crossing branch from 524868e to afa8267 Compare April 6, 2021 02:40
jdavies-st
jdavies-st commented Apr 6, 2021
View reviewed changes
jwst/resample/resample_spec.py
ra_slice = ra[:, lam_center_index]
dec_slice = dec[:, lam_center_index]
# wrap RA if near zero
ra_center_pt = np.nanmean(wrap_ra(ra_slice))
Copy link
Collaborator Author

@jdavies-st jdavies-st Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was the bug. For RA~0, the mean of the RA slice was the mean of zeros and 359, so an output frame centered at RA~180 deg.

@nden
Copy link
Collaborator

nden commented Apr 6, 2021

LGTM.
Since there'e nothing specific to cube_build in wrap_ra, I'd suggest moving it to a more central place so it can be reused.

@jdavies-st jdavies-st requested a review from hbushouse April 6, 2021 14:12
@jdavies-st
Copy link
Collaborator Author

jdavies-st commented Apr 6, 2021 •
edited
Loading

Yeah, I'd like to do a follow-up PR that does a bit of refactoring to wrap_ra, replacing it with a function that uses astropy.coordinates.SkyCoord midpoint functionality. As it is, we only deal with the discontinuity at RA=0, but there's also a discontinuity at zenith. Instead of reinventing the wheel, we should just be using SkyCoord objects to deal with it correctly. I'll file a separate issue. See #5932.

@stscijgbot-jp stscijgbot-jp mentioned this pull request Apr 6, 2021
Closed
hbushouse
hbushouse approved these changes Apr 6, 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. So this only affected resample_spec and not resample (for images)?

@jdavies-st
Copy link
Collaborator Author

@hbushouse right, it was only in the build_interpolated_output_wcs() function used for spectral slits. For imaging, we use the footprint() of each WCS and a separate wcs_from_fiducial() function from gwcs.

@jdavies-st jdavies-st force-pushed the JP-1829-resample-spec-zero-crossing branch from 789cc0f to c4c5fe0 Compare April 12, 2021 15:18
@jdavies-st
Copy link
Collaborator Author

jdavies-st commented Apr 12, 2021 •
edited
Loading

Added one last commit to fix a bug that the NIRSpec MOS lev3 regression tests revealed.

@jdavies-st jdavies-st merged commit 434c540 into spacetelescope:master Apr 12, 2021
@jdavies-st jdavies-st deleted the JP-1829-resample-spec-zero-crossing branch April 12, 2021 16:00
@stscijgbot-jp stscijgbot-jp mentioned this pull request Apr 12, 2021
Closed
kmacdonald-stsci pushed a commit to kmacdonald-stsci/jwst that referenced this pull request Apr 13, 2021
@jdavies-st @kmacdonald-stsci
JP-1829: Fix resample_spec output size from input images crossing RA=0 (
b12befc 
spacetelescope#5929)

* Output i2d in image2 for NIRCam regtest

* Fix build_interpolated_output_wcs for WCS near RA=0

* Update tests

* Fix test warnings

* Use np.append instead of looping

Separated the OLS and GLS functionality into their own file.

Removing an unused function and refactoring the  function.

Break  into smaller functions.

Making flake8 style changes.

Updating docstrings for new functions.

Updating the comments.
kmacdonald-stsci pushed a commit to kmacdonald-stsci/jwst that referenced this pull request Apr 14, 2021
@jdavies-st @kmacdonald-stsci
JP-1829: Fix resample_spec output size from input images crossing RA=0 (
db5e89d 
spacetelescope#5929)

* Output i2d in image2 for NIRCam regtest

* Fix build_interpolated_output_wcs for WCS near RA=0

* Update tests

* Fix test warnings

* Use np.append instead of looping

Separated the OLS and GLS functionality into their own file.

Removing an unused function and refactoring the  function.

Break  into smaller functions.

Making flake8 style changes.

Updating docstrings for new functions.

Updating the comments.
hbushouse pushed a commit that referenced this pull request Apr 21, 2021
@kmacdonald-stsci @jdavies-st
JP-2036: refactor fit_next_segment (#5953)
d26b6f0 
* JP-1829: Fix resample_spec output size from input images crossing RA=0 (#5929)

* Output i2d in image2 for NIRCam regtest

* Fix build_interpolated_output_wcs for WCS near RA=0

* Update tests

* Fix test warnings

* Use np.append instead of looping

Separated the OLS and GLS functionality into their own file.

Removing an unused function and refactoring the  function.

Break  into smaller functions.

Making flake8 style changes.

Updating docstrings for new functions.

Updating the comments.

* Fixing bug found with 'None * 22'.

* Updating tests.

* Making style changes due to flake8 failures.

* Changing comment to a test to clarify that since there are only two groups in the data setting the first and last groups to DO_NOT_USE means setting all groups to DO_NOT_USE.

* Removing unnecessary comment in ols_fit.py and adding clarifying comment to tests.

Co-authored-by: James Davies <[email protected]>
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

@jemorrison jemorrison Awaiting requested review from jemorrison

@nden nden Awaiting requested review from nden

Assignees
No one assigned
Labels
resample_spec
Projects
None yet
Milestone
Build 7.8
Development

Successfully merging this pull request may close these issues.

resample_spec output frame is spatially too small
3 participants
@jdavies-st @nden @hbushouse