Normunpol batch raytrace#225
Conversation
|
Btw, sorry if I am not following procedure very well for the pull request. I am not too familiar with it yet. |
There was a problem hiding this comment.
Very nice work, thanks! I agree that zospy.analyses.raysandspots is the best place for this feature, although it is strictly spoken not an analysis.
Btw, sorry if I am not following procedure very well for the pull request. I am not too familiar with it yet.
No problem, we are happy to guide you through the process!
A few general comments:
- Read the contributing guidelines for instructions on how to lint, format and test ZOSPy. Several points in this review could have been discovered and fixed by using a linter. Furthermore, the linting and formatting checks are required to pass in order to merge the PR.
- For me, this analysis only works in standalone mode. In extension mode, I get a
RemotingException. Is this expected? If so, the analysis should check for the connection mode and raise a clear exception in extension mode. - Tests are still missing. The issues with settings validation (where px=[2] is outside the allowed range, but still accepted) could have been caught earlier by adding tests.
- It would be better to manage the batch ray trace tool with a context manager, to ensure the tool will be closed even after an exception. We can consider providing a utility function for this in a separate PR.
I agree the warning about the comment in the raytrace analysis is annoying; I always suppress it in my code, but thanks for addressing this. Since this is a separate change, it belongs in a separate PR. This time we'll accept this, but next time please open separate PRs for different changes.
Co-authored-by: crnh <30109443+crnh@users.noreply.github.com>
|
Thanks a lot for your review! I tried implementing your points as well as I could. Just a comment about a change: I am not very happy with using Btw, I think it is expected that the batch raytrace does not work in extension mode because it is not really an analysis that can be run by itself from within OpticStudio. |
Yes, I noticed that as well. The best way to allow NumPy arrays as well is probably typing these attributes with a union of Sequence and NDArray. This requires a custom validator to ensure additional constraints are applied to the array items as well. |
|
Something that could be cool to implement in the future (not a feature request): Zemax batch tracing, needs you to add the rays one by one, this is a huge slowdown due to python loops and API overhead, retrieval of traced rays suffers from this as well. There is the option to outsource the ray adding and retreival to C#: https://optics.ansys.com/hc/en-us/articles/42661765866899-Batch-Processing-of-Ray-Trace-Data-using-ZOS-API-in-MATLAB-or-Python (the example is bugged by the way, but the idea is there) With this extra wrapper you could pass and retrieve the rays via numpy arrays. If this would be implemented in ZOSPy, the project would need to ship the C# DLL though (question is, where to put it, as git does not like binaries), cross platform should not be an issue, as Zemax is Windows only anyways. |
…' into normunpol-batch-raytrace
|
I managed to transfer the batch raytrace over to Regarding unit tests, I am having difficulties running @Wellidontcare: Sounds interesting. For my use cases, the python loop does not really cause a huge slowdown but I can imagine there are applications where this matters. Would be cool to implement in the future. |
Can you let us know what goes wrong, so we can maybe help you and if applicable update our contribution guidelines? If you don't want to discuss this as part of this PR, please drop us a message at zospy@mreye.nl. If the issue is related to |
Not all usages of _deserialize_zospy_class correctly specified the module
|
@andibarg I added unit tests for the batch ray trace tool and updated the changelog. Once the tests pass for all supported Python versions, this one is good to go! |
Proposed change
1. NormUnpol Batch Raytrace
I added the NormUnpol batch raytrace as a new analysis under
raysandspots- hope this is the right place for it. It can be used, for instance, to create spot diagrams (see the official examples).The batch ray trace is technically not an OpticsStudio analysis but I thought we could perhaps still treat it as such. Note that I could not add the settings to the RayTrace analysis under
run_analysisas inself.analysis.Settings.Hx = self.settings.hxbecause it does not accept lists. Perhaps there is a workaround or better solution to this.Unfortunately, I cannot run/provide Jupyter notebook examples at the moment. Instead, I prepared this MWE:
2. Removed warning from SingleRayTrace when surface comment is missing
There is a warning when running the single ray trace in the Double Gauss example. This is simply because some surfaces in the .zmx file do not have a comment. Even though I understand the reason for the warning, I thought it would be nice to avoid the warning in this case.
Type of change
Additional information
Related issues
Checklist
hatch test -a).If you updated an example:
hatch run all-examples).If you contributed an example: