search

mperrin / poppy

Physical Optics Propagation in Python
BSD 3-Clause "New" or "Revised" License
173 stars 40 forks source link

Transformations of WavefrontError and subclasses don't do what is expected #229

Closed corcoted closed 6 years ago

corcoted commented 7 years ago

The shift_x, shift_y, and rotation don't behave well for the WavefrontError subclasses. These are inherited from AnalyticOptics to do coordinate transformations on the optics.

For example, trying ZernikeWFE(radius=1*u.m, coefficients=[0,1,0], shift_x=0.6).display(what='opd') gives me this:

image

The aperture seems to be chopped to an American football shape and the center of the opd pattern is at the origin. The expected behavior is to have a circular aperture and its opd shifted over by 0.6 in the x direction.

(The transmission is 1 everywhere.)

mperrin commented 7 years ago

That's definitely a bug. But luckily not a hard one to track down. The shifts weren't getting passed through to the call to the zernike basis function, so the above is an unshifted tilt intersected with a shifted circular mask around it. The reason the shifts weren't getting passed through was related to the use of the zernike.cached_zernike1 caching version for better speed on repeated invocations; that cache isn't set up to handle offset coordinates. I added a check for if there's any offset or rotation, and if so now it will call the regular zernike.zernike1 instead, and with the offset coordinates. Will be slightly slower, but more correct! Seems to be working properly in my tests here. Try now on the latest master and see if it's working better for you.

Meanwhile this reminds me of a related question, whether the transmission should be masked also. A while ago we changed the ThinLens class to make it a subclass of CircularAperture such that the transmission is only nonzero over that aperture. Could do similarly with ZernikeWFE since the Zernike are only valid over the unit circle, but we haven't yet. As I recall the reasoning was that other WFE classes could have different valid apertures (or be valid over any aperture without bounds as in the case of the SineWaveWFE class for instance), so it was more consistent for the API to leave them all defining OPDs but not constraining the transmission. I think that makes sense, but could be open to hearing other arguments. This bug reminded me of the question because ZernikeWFE is using a call to CircularAperture in its internals for masking the OPD to only the valid region, but it doesn't similarly mask the transmission, so it's not entirely consistent.

mperrin commented 7 years ago

figure_1

mperrin commented 7 years ago

I believe this issue was resolved back in July with commit 3799489. @corcoted if you have time, can you let me know if this is all working correctly for you now? If so I'll go ahead and close this issue.

mperrin commented 6 years ago

Stale issue, resolved a year ago. Going to close this now.