8000 JP-1585: Wrap first angle at 360 in forward V2V3 to sky and at 180 for inverse. by mcara · Pull Request #5206 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-1585: Wrap first angle at 360 in forward V2V3 to sky and at 180 for inverse. #5206

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
Merged
merged 2 commits into from
Aug 27, 2020
Merged

JP-1585: Wrap first angle at 360 in forward V2V3 to sky and at 180 for inverse. #5206

merged 2 commits into from
Aug 27, 2020

Conversation

mcara
Copy link
Member
@mcara mcara commented Jul 29, 2020 •
edited
Loading

Fixes #5148 / JP-1585. Improves upon #5185 by restoring wrap angle for V2 to 180 degrees so that inversion polynomials that take from detector to V2V3 frames now can work as designed.

Closes #5195
Closes #5185

This PR replaces V23ToSky model with "standard" models from astropy and gwcs.

@mcara mcara requested review from nden and hbushouse July 29, 2020 12:08
@mcara mcara self-assigned this Jul 29, 2020
hbushouse
hbushouse reviewed Jul 29, 2020
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.

I will have to defer to the expertise of @nden for approval of this approach.

@hbushouse
Copy link
Collaborator

If this approach is approved, does it supersede both #5195 and #5196 ?

@mcara
Copy link
Member Author
mcara commented Jul 29, 2020

Yes, it supercedes #5185 and #5195. It also removes the need for custom model V23ToSky. However, I did not remove V23ToSky because I am not sure how to do this safely (so that it works with old files) @nden

@mcara mcara force-pushed the fix-niriss-angle-wrapping-no-custom branch 2 times, most recently from 6803393 to 4ab7cd1 Compare July 29, 2020 14:15
@nden nden mentioned this pull request Aug 10, 2020
Closed
@mcara mcara force-pushed the fix-niriss-angle-wrapping-no-custom branch from 4ab7cd1 to bf12ab1 Compare August 26, 2020 00:36
@jdavies-st jdavies-st added this to the Build 7.6 milestone Aug 26, 2020
jdavies-st
jdavies-st reviewed Aug 27, 2020
View reviewed changes
jwst/lib/set_telescope_pointing.py Outdated
Comment on lines 551 to 552
angles = [-v2_ref_deg, v3_ref_deg, -roll_ref, -dec_ref, ra_ref]
axes = "zyxyz"
Copy link
Collaborator
8000

Choose a reason for hiding this comment

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

These two lines need to be removed, as angles and axes are no longer used.

@mcara mcara force-pushed the fix-niriss-angle-wrapping-no-custom branch from 1c23373 to bbb67b0 Compare August 27, 2020 18:25
jdavies-st
jdavies-st approved these changes Aug 27, 2020
View reviewed changes
Copy link
Collaborator
@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

This looks good!

@nden
Copy link
Collaborator
nden commented Aug 27, 2020

The failures in the code style check need to be fixed but otherwise looks good.
We can close delete now the branch move-transforms-out-of-jwst, the same changes are implemented in this PR.

@mcara mcara force-pushed the fix-niriss-angle-wrapping-no-custom branch from 168461a to b4ce120 Compare August 27, 2020 19:22
@mcara
Copy link
Member Author
mcara commented Aug 27, 2020

PEP8 fixed in the last commit (squashed onto previous one).

This was referenced Aug 27, 2020
Closed
Closed
@jdavies-st jdavies-st changed the title Wrap first angle at 360 in forward V2V3 to sky and at 180 for inverse. JP-1585: Wrap first angle at 360 in forward V2V3 to sky and at 180 for inverse. Aug 27, 2020
@codecov
Copy link
codecov bot commented Aug 27, 2020

Codecov Report

Merging #5206 into master will decrease coverage by 0.00%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5206      +/-   ##
==========================================
- Coverage   52.40%   52.39%   -0.01%     
==========================================
  Files         406      406              
  Lines       36480    36475       -5     
  Branches     5645     5646       +1     
==========================================
- Hits        19117    19111       -6     
- Misses      16183    16184       +1     
  Partials     1180     1180
Flag Coverage Δ
#unit 52.39% <94.73%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jwst/assign_wcs/pointing.py 86.32% <93.33%> (+0.61%) ⬆️
jwst/lib/set_telescope_pointing.py 83.24% <100.00%> (-0.43%) ⬇️
jwst/assign_wcs/util.py 50.77% <0.00%> (-0.26%) ⬇️

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 ca84d23...b4ce120. Read the comment docs.

@jdavies-st jdavies-st merged commit 7ff76b2 into spacetelescope:master Aug 27, 2020
@stscijgbot-jp stscijgbot-jp mentioned this pull request Sep 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbushouse hbushouse hbushouse left review comments

@nden nden nden approved these changes

@jdavies-st jdavies-st jdavies-st approved these changes

Assignees

@mcara mcara

Labels
assign_wcs bug set_telescope_pointing
Projects
None yet
Milestone
Build 7.6
Development

Successfully merging this pull request may close these issues.

Negative RAs sometimes returned from assign_wcs
4 participants
@mcara @hbushouse @nden @jdavies-st
0