Fft mtf map

[laser-axel]

Proposed change

Added methods OpticStudioSystem.LDE.SurfaceByComment() and OpticStudioSystem.LDE.SurfaceByRegex() to keep scripts and tools functioning when a surface is added to the system Added zospy.analyses.mtfclasses to have some added features for the FFT-MFT and FFT-MFT-map for plotting in pyplo

Type of change

• [x] Example (a notebook demonstrating how to use ZOSPy for a specific application)
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New analysis (a wrapper around an OpticStudio analysis)
• [ ] New feature (other than an analysis)
• [ ] Breaking change (fix/feature causing existing functionality to break)
• [ ] Code quality improvements to existing code or addition of tests
• [ ] Documentation (improvement of either the docstrings or the documentation website)

Additional information

• Python version: 3.11.8
• OpticStudio version: 2023 R1.03

Related issues

It is common practise for me to add environment variables to my windows user account to keep certain settings as the path to the dev-checkout of zospy persistent. I can change this to whatever you think is more convenient

Checklist

• [ ] I have followed the contribution guidelines
• [ ] The code has been linted, formatted and tested locally using tox.
• [ ] Local tests pass. Please fix any problems before opening a PR. If this is not possible, specify what doesn't work and why you can't fix it.
• [ ] I added new tests for any features contributed, or updated existing tests.
• [ ] I updated CHANGELOG.md with my changes (except for refactorings and changes in the documentation).

If you contributed an analysis:

• [x] I did not use AttrDicts for the analysis result data (please use dataclasses instead).

If you contributed an example:

• [x] I contributed my example as a Jupyter notebook.

[crnh]

Hi @laser-axel,

Thanks for your contributions! These are a lot of different changes, and it is better to open separate pull requests for this:

• One for the new FFT MTF map analysis;
• One for the new functions for the Lens Data Editor.
• No new PR for the changes to zospy.api.config; we kindly request you to remove these changes from the PR and open an issue instead. We still need to discuss a proper solution and want to have this problem and its solution properly recorded on GitHub.

Furthermore, here are some first points for improvement:

• As documented in the contributing guidelines, we have some styling requirements. Please adhere to these style guidelines.
• Please undo the changes to .gitignore and README.md. (We are not against changing .gitignore, but these changes look quite specific to your setup).
• Please remove .ipynb_checkpoints.
• The new analysis does not follow the structure of the other analyses. Please stick to this structure; it should be possible to run the analysis with a single function call. The new analysis should be added in zospy/analyses/mtf.py and not in a new file (zospy/analyses/mtfclasses.py).
• We understand why you implemented some plotting functionality, but ZOSPy is not a plotting library. We deliberately chose to not include plotting functions; one reason for this choice is explained here.
• Helper functions should not be added to zospy.zpcore.OpticStudioSystem, but in zospy.functions. Note that the functionality of SurfaceByComment is already implemented in zospy.functions.lde.find_surface_by_comment. The other functions look like nice additions to ZOSPy that we are willing to include after some modifications.

We are looking forward to your new pull requests, and don't hesitate to ask us any questions if something is unclear!

[laser-axel]

I totally agree to your first 3 bullets you mention at "points for improvement". Also I did not get tox and pytest to run the existing tests, so far. Regarding the plotting fuctions. Technically I did just added a colormap and a list of line-colors as attributes. All the actual plotting is done in the .jpynb. Anyway, I can remove that.

But I don't get the idea behind your "no classes and methods, use function calls instead"-appproach. Not intented as flame! I still like what you have done so far. Sorry. but that is absolutely contrary to my mindset. When a student in the lab comes with new piece of code with a call to a function with an object reference as 1st parameter and dozens of other parameters, I would say: "The object referenced should carry all those parameters and have a method to do that." And that`s the way the ZOSAPI originally works. There is certainly room for improvements and enhancements, but I don't see any benefits in chaning general way of organizing the myriads of features of Optic Studio. IMHO from going from the purely OO-approach in the ZOSAPI to a set of function calls witch object reference scheme is a kind of step backward rather than forward.

Was a nice try. I did enjoy trying get me idea here. Learned a lot. No hard feelings on my side!

Best regards, Axel

[crnh]

Regarding the plotting fuctions. Technically I did just added a colormap and a list of line-colors as attributes. All the actual plotting is done in the .jpynb. Anyway, I can remove that.

Everything related to colors and plot layout is very dependent on personal preferences. As stated in the referenced discussion, to make these functions suitable for wider use, they need a lot of customization options and proper unit tests. In the end, this adds a maintenance burden that we are currently not willing to bear.

If there appears to be a high demand for plotting functions in ZOSPy, then we are of course willing to add these. Even then, this functionality should be separated from the analysis functions, probably in a separate module.

But I don't get the idea behind your "no classes and methods, use function calls instead"-appproach.

When a student in the lab comes with new piece of code with a call to a function with an object reference as 1st parameter and dozens of other parameters, I would say: "The object referenced should carry all those parameters and have a method to do that."

I understand your preference for an object-oriented approach and in most cases this is indeed the right thing to choose. We also considered an object-oriented interface for the analyses, but in the end went for a procedural interface for a few reasons:

• An object-oriented interface does not really offer a benefit over the "raw" ZOS-API analysis interface. If you want to configure and run analyses in an object-oriented way, you may be better off using the ZOS-API directly.
• ZOSPy seems to be used a lot in interactive environments like Jupyter notebooks, and this is also how we mainly use it ourselves. In this context, a procedural interface makes sense.
• Analyses work like functions: you put in some parameters and get a result. Unlike for example optimization operands, OpticStudio analyses are relatively independent from each other.

Regarding the number of parameters: wherever sensible, the number of parameters should be kept as small as possible. However, in the (scientific) Python ecosystem, it is not uncommon to have functions with a lot of parameters (see for example Matplotlib).

"The object referenced should carry all those parameters and have a method to do that."

Are you referring to the oss object that is passed to every analysis? I understand your concerns here, but (monkey) patching the objects from the ZOS-API is less elegant and results in a less maintainable solution. We may consider making the oss parameter optional and obtain the primary system by default, so it is not necessary to always supply the oss argument. However, this is more error-prone.

I did enjoy trying get me idea here.

Do you mean one of the purposes of this PR was to propose a new analysis interface? We do welcome such proposals, but please do this in a discussion as it boils down to an architectural change in ZOSPy.

Was a nice try.

Do you want to continue working on this PR, or should it be closed?

No hard feelings on my side!

Same for us!

[laser-axel]

This stems from the discussion the zemax communitz forum. I drew the line between added feature and architectural change somewhere else.