New subaperture optic ---NOT READY TO MERGE.--- #185
Conversation
poppy/sub_sampled_optics.py
Outdated
|
|
||
| def __init__(self, | ||
| dimensions = (2,2), | ||
| optic_array = np.array([[CircularAperture(radius=2.,planetype=PlaneType.pupil), |
There was a problem hiding this comment.
Watch out for mutable default arguments! (see http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument for more details)
From a software-engineering perspective rather than an optics perspective, I would say documentation and testing. In particular, an example optical system in a notebook would be nice to play with (or just look at) for evaluating the ergonomics of the API. Also, since this models a physical system, a test case that, e.g., shows it really does recover the right Zernikes (to some precision) when used as a SH sensor would be a useful sanity check.
See my line comments for things that stood out.
Hmm. optics.py is already almost 2000 lines, so I think it should be its own file. The existing optics in POPPY are exposed in the main namespace through judicious use of (It's possible there's a sensible way to split up optics.py into a package with submodules too, but that would probably be a separate PR.) |
| self.optic_array=optic_array | ||
| self.crosstalk=crosstalk | ||
| if crosstalk: | ||
| raise ValueError("CROSS TALK NOT IMPLEMENTED <YET>") |
There was a problem hiding this comment.
Even better, use NotImplementedError
|
@josePhoenix, thanks for feedback. Yes, I thinking refactoring into some helper functions is a good idea. I have to think about the mutable arguments, thanks for the warning. you mentioned wanting to see a notebook, do you want to see something beyond the notebook linked above? I put it in the gist because I didn't want to clog up the poppy git history with lots of iterations of notebook graphics. |
|
Hahah, I totally overlooked that link. (Maybe I was using my phone?) That's great! |
|
@douglase is this optic still not ready to merge? I'm hoping to get a poppy release out soon, and my default plan is to leave this out unless you tell me you'd prefer otherwise. In which case, I see there is now a merge conflict with more recent edits in poppy_core that would need to get resolved... |
|
I am still working on test cases for this, please leave it out. If you
prefer to not have the clutter I can close the PR.
…On Mon, Jul 10, 2017 at 17:05 Marshall Perrin ***@***.***> wrote:
@douglase <https://github.com/douglase> is this optic still not ready to
merge? I'm hoping to get a poppy release out soon, and my default plan is
to leave this out unless you tell me you'd prefer otherwise. In which case,
I see there is now a merge conflict with more recent edits in poppy_core
that would need to get resolved...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#185 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA-nn6fUciKG7UPJSje-HkR8ogaLBbGdks5sMq4cgaJpZM4J5w6w>
.
|
|
No bother about the "clutter" at all! I'm perfectly happy to leave the PR open as-is. Just wanted to ask you to make sure that was the right thing to do. Thanks! |
Here's my first draft of a Shack-Hartmann simulator. Not a finished product, just getting it out there for feedback early.
I've tried to keep it general and make it a "sub_aperture" class which could feed into an IFS or other instrument that samples a wavefront via a regular grid.
Here's a demo notebook showing sampling a wavefront, generating an array of PSFs and a modal retrieval of the input wavefront (this is my first foray into SH wavefront sensing, so it's not at all optimized but it does seem to recover the right Zernikes): https://gist.github.com/douglase/9f70aaffbd683f3af5edf805847a444e
Things needed before merging:
Things that raise a Not Implemented:
Community questions: