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

Requested feature - added option to return complex image plane wavefront #234

Closed
mperrin opened this issue Aug 27, 2018 · 9 comments
Closed

Comments

@mperrin
Copy link
Collaborator

mperrin commented Aug 27, 2018

Issue by douglase
Friday Aug 18, 2017 at 22:33 GMT
Originally opened as mperrin/poppy#234


suggested by @jjlumbres, return the final complex plane with calc_psf, without returning all the other intermediate wavefront planes, saving memory.

w/ tests.


douglase included the following code: https://github.com/mperrin/poppy/pull/234/commits

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by coveralls
Saturday Aug 19, 2017 at 15:10 GMT


Coverage Status

Coverage increased (+0.03%) to 65.432% when pulling 9fbb14d on douglase:complexpsf into 6b15954 on mperrin:master.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by coveralls
Saturday Aug 19, 2017 at 15:49 GMT


Coverage Status

Coverage increased (+0.03%) to 65.432% when pulling 706e112 on douglase:complexpsf into 6b15954 on mperrin:master.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by mperrin
Tuesday Aug 22, 2017 at 18:07 GMT


This looks good, and I can see why this feature is a good idea, sure.

Semi-relatedly, this PR made me realize about an API inconsistency: we use retain_intermediates in poppy_core.py but return_intermediates in instrument.py. I wonder if it's worth doing something to make that more consistent. But this almost certainly isn't the time to do that right now in any case.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by coveralls
Tuesday Aug 22, 2017 at 18:40 GMT


Coverage Status

Coverage increased (+0.03%) to 65.432% when pulling 88d101d on douglase:complexpsf into 6b15954 on mperrin:master.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by douglase
Tuesday Aug 22, 2017 at 19:03 GMT


(@mperrin, the square was because I used max, I didn't want an array with a bunch of negative numbers and one zero to pass the test.)

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by coveralls
Tuesday Aug 22, 2017 at 19:19 GMT


Coverage Status

Coverage increased (+0.03%) to 65.432% when pulling 114221e on douglase:complexpsf into 6b15954 on mperrin:master.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by douglase
Monday Nov 13, 2017 at 20:56 GMT


Is further revision required to merge this PR?

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by mperrin
Tuesday Nov 14, 2017 at 01:43 GMT


Eh, mostly just your reminding me. Thanks :-)

I sort of vaguely thought there was some check that wasn't passing, but that doesn't appear to be the case. I must have confused it with a different PR I guess. Happy to merge it now.

@mperrin
Copy link
Collaborator Author

mperrin commented Aug 27, 2018

Comment by mperrin
Tuesday Nov 14, 2017 at 02:09 GMT


Huh. Despite both tests passing, merging this made the Travis build of master fail, in the step where it builds the docs. Huh? Going to try to kick it and see if re-running that job does any better

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

1 participant