search

spacetelescope / webbpsf

James Webb Space Telescope PSF simulation tool
https://webbpsf.readthedocs.io
BSD 3-Clause "New" or "Revised" License
115 stars 61 forks source link

Suspect rotation matrix in NIRISS_GR700XD_Grism.get_opd() #357

Open Russell-Ryan opened 4 years ago

Russell-Ryan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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...