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

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

Closed
corcoted opened this issue Jul 12, 2017 · 4 comments
Closed

Comments

@corcoted
Copy link
Contributor

corcoted commented Jul 12, 2017

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
Copy link
Owner

mperrin commented Jul 12, 2017

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
Copy link
Owner

mperrin commented Jul 12, 2017

figure_1

@mperrin
Copy link
Owner

mperrin commented Aug 28, 2017

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
Copy link
Owner

mperrin commented Apr 3, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants