Merge sim branch to main #52
Conversation
+ other cleanup
- Swapped None and 0 for phase_steps. - Saved the old phase calibration.
+ Tied together slm.spot_radius_kxy() and fs.get_farfield_spot_size() + Enable simulation.ipynb example
| img = img.get() | ||
| img = self.exposure * img | ||
|
|
||
| if plot: |
There was a problem hiding this comment.
Plotting should probably either be removed or added to the superclass.
There was a problem hiding this comment.
Moved to a staticmethod under superclass. @ichristen Request your re-review.
| self.write(None) | ||
|
|
||
| def _write_hw(self, phase): | ||
| """Updates SLM.display to implement various physical artifacts of SLMs.""" |
There was a problem hiding this comment.
It might be interesting to add SLM blur. Should this be here or in the simulated camera?
There was a problem hiding this comment.
Physical effects can be added as needed.
| img = toolbox.unpad(img,self.shape) | ||
| if mp != np: | ||
| img = img.get() | ||
| img = self.exposure * img |
There was a problem hiding this comment.
I made some slight formatting and variable name modifications to this file. My major comment is that this is probably a good opportunity to add in readout noise and dark current.
There was a problem hiding this comment.
Now added in 99b7c5b.
| Defaults to 0 (i.e., aligned with the SLM). | ||
| offset : tuple | ||
| Lateral displacement (in `basis` units) of the camera center from `slm`'s | ||
| optical axis. If ``None``, defaults to 0 offset. |
There was a problem hiding this comment.
It might be worth considering making a helper function for constructing affine transformations with offset, rotation, and scale. I think simulated would be simpler and more general if it used an affine transformation, but user-friendliness can still be maintained with a helper function.
There was a problem hiding this comment.
@ichristen Are you suggesting to feed in the same (M,b) from "kxy"->"ij" transformations, apply the inverse, and then do the interpolation in "kxy" space? Or to apply a forward affine transformation to map "ij" -> "xy"?
There was a problem hiding this comment.
Yeah, probably the same {"M", "b"}, if that's the option that is most uniform with the rest of the package.
There was a problem hiding this comment.
Looking at this more, the f_eff scale term is a bit of a pain for the user to deal with in the way I was thinking. I think your implementation is the right approach, with just handling everything inside.
There was a problem hiding this comment.
Done! Needs hardware testing and a digital twin method now
There was a problem hiding this comment.
Completed; see simulation tutorial for sample uses.
-More cleanup required -Hardware testing required -Removed mask with border (was causing off by one issues when a large mask was used)
Now hardware tested. Co-Authored-By: Chris Panuski <12805891+cpanuski@users.noreply.github.com>
Simplifies source setting, plotting, and tracking. Need to debug documentation (probably added a few issues) and get autoformatters on same page.
|
Also, is there a reason to use |
Almost all overhead is in the initialization; the Hologram wrapper cleans up the FFTs and allows expansion towards different kernels. |
Also update logo.
In response to #55; uses the same format as SLM hardware interface methods. + Limited curve fitting steps in wavefront calibration
+ associated debugging of sim mode. Need to check if I broke "fwd" mode of transform_grid Co-Authored-By: Chris Panuski <12805891+cpanuski@users.noreply.github.com>
|
Hardware-integration (i.e. cloning from hardware to simulation) completed and all initial review comments addressed. @ichristen request review to merge PR. |
|
Closing in favor of #69. |
Basic features implemented. Open to delaying merge to add in any other features you'd like to see.