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

Suspect rotation matrix in NIRISS_GR700XD_Grism.get_opd() #25

Open
Russell-Ryan opened this issue Jun 8, 2020 · 2 comments
Open

Suspect rotation matrix in NIRISS_GR700XD_Grism.get_opd() #25

Russell-Ryan opened this issue Jun 8, 2020 · 2 comments
Labels
bug Something isn't working

Comments

@Russell-Ryan
Copy link

HI... I was looking at optics.py with Robel in preparation of the work we will be doing for Roman, and I spotted lines 481 and 482 that look suspect. Based on the lines, I'm guessing they are to implement a rotation in 2d space of some (x,y) pairs (perhaps a vector of pairs). But, it appears that line 481 will compute a new x-coordinate and line 482 will use the new x-coordinate, which is probably not what is intended. I would've expected the lines to be something like:

xp = cosx-siny
yp = sinx+cosy
x,y=xp,yp

Or maybe the '-' is reversed, depending on the sense of the rotation. I'm still only a novice at python, so maybe the interpreter knows how to do this operation? I guess for small angles (<~0.1 deg) this probably doesn't matter, but it looked odd.

@mperrin mperrin changed the title Suspect rotation matrix Suspect rotation matrix in NIRISS_GR700XD_Grism.get_opd() Jun 8, 2020
@mperrin mperrin added the bug Something isn't working label Jun 8, 2020
@mperrin
Copy link
Collaborator

mperrin commented Jun 8, 2020

Oh good catch @Russell-Ryan and @robelgeda, at first glance this does look like a bug. And I see there's another instance of the same thing in the get_transmission method as well as in get_opd for the NIRISS grating.

Both of these should be refactored to use the poppy method AnalyticOpticalElement.get_coordinates() which (correctly) implements the rotation transformation and more (shifts, tilts, etc).

@mperrin
Copy link
Collaborator

mperrin commented Jun 8, 2020

BTW, it's probably worth mentioning that the I've had some lingering concern on the NIRISS SOSS grating model, since I'm not 100% sure we ever fully validated how well the output PSFs correspond to real test data from the hardware. So I am not entirely surprised to see that something slipped through the cracks here...

@BradleySappington BradleySappington transferred this issue from spacetelescope/webbpsf Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants