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

Refactor ZOSPy core and remove deprecated functions #107

Merged
merged 19 commits into from
Feb 7, 2025

Conversation

crnh
Copy link
Collaborator

@crnh crnh commented Jan 6, 2025

Proposed change

zpcore has been refactored and deprecated methods in zospy.zpcore and zospy.functions have been removed.

Notable changes:

  • ZOS is now a singleton. Calling zospy.ZOS for the second time will raise a warning and return the existing instance. I used a weakref.WeakValueDictionary to store the existing instance. Because the dictionary will at most contain a single value, using a weakref.ref object would be possible as well, but this would require explicitly setting the value to None on initialization and deletion. The current implementation will automatically empty the dictionary. Alternatively, a weakref.WeakSet would work as well, but retrieving a single value from a dictionary is a bit easier than retrieving it from a set. Please explain which option you prefer as part of the review.
  • ZOS has a new classmethod get_instance that allows to get an existing instance if available. Unlike __init__, calling this method will not create a new instance. This method could for instance be used to make the oss parameter for analyses optional, automatically using the primary system of a connected instance if oss is not specified.
  • OpticStudioSystem._ZOS was renamed to ZOS, making it public. We use it quite a lot outside OpticStudioSystem itself, e.g. to check for the OpticStudio version, so I think it makes sense to make it a public attribute. Alternatively, we may opt for adding a separate version attribute to OpticStudioSystem. Please comment on this as part of the review.
  • OpticStudioSystem.ZOS is now a weakref.proxy object, so removing the ZOS instance (e.g. with del zos) will work without first removing the OpticStudioSystem instance. Previously, oss blocked the garbage collector from deleting ZOS instances if there were still OpticStudioSystem instances around, see Deleting the ZOS instance doesn't (always) work #61.

Type of change

  • 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.12
  • OpticStudio version: 24R2

Related issues

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:

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

If you contributed an example:

  • I contributed my example as a Jupyter notebook.

crnh added 6 commits December 23, 2024 17:05
Without this assignment, the weak ref in _instances is immediately garbage collected.
Because OpticStudioSystem._ZOS is not only used internally, it is better to make it public. Furthermore, it is now a weak reference to the ZOS instance, to prevent oss from blocking the termination of ZOS.
@crnh crnh requested review from LucVV and jwmbeenakker January 6, 2025 10:53
@crnh
Copy link
Collaborator Author

crnh commented Feb 6, 2025

To do:

  • Add __all__ in modules that are part of the public API

@crnh crnh marked this pull request as ready for review February 7, 2025 14:45
@crnh crnh requested a review from LucVV February 7, 2025 14:45
@crnh crnh added this to the v2.0.0 milestone Feb 7, 2025
@crnh crnh merged commit c2c17eb into v2.0.0 Feb 7, 2025
13 checks passed
@crnh crnh deleted the crnh/v2.0.0/refactor_core branch February 7, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants