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

Reorganize TEMController #99

Merged
merged 15 commits into from
Nov 7, 2024

Conversation

viljarjf
Copy link
Contributor

@viljarjf viljarjf commented Nov 6, 2024

Hello,
This reorganizes the TEMController module:

  • The TEMController class, which was in the TEMController file, in the TEMController module, is now moved to the controller.py-file in the main folder.
  • The TEMController folder is renamed to microscope.
  • Specific instrument interfaces are moved to microscope/interface.
  • Deflectors, lenses ect. are moved to microscope/components.

Some of the files have been given simpler names, and some functions are renamed as well.
To ensure backwards compatibility, I added a deprecation decorator to warn users that the way things are imported is changing. See e.g. https://github.com/viljarjf/instamatic/blob/efa311ecc536d2724c71207f2fd2cb3c1462ddad/src/instamatic/microscope/microscope.py#L66-L71

Here is a summary:
image

Opening this as a draft to hear thoughts and suggestions before I go through and update all the documentation, and add more deprecation warnings.
I am unsure about the components-folder, since they are only used by the TEMController-class, which is now placed in a file outside of the microscope-folder. However, placing controller.py inside the microscope-folder seems unfitting, seeing how important the controller class is.
Additionally, should the files not part of the reorganization be left unchanged for now? They will show the deprecation warning, but this reduces the amount of files part of this PR significantly.

@viljarjf viljarjf requested a review from stefsmeets November 6, 2024 13:58
Copy link
Member

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Already looks pretty good to me. I think this is a nice step forward in the organisation of the code, and I'm also happy with the better class / function names.

Opening this as a draft to hear thoughts and suggestions before I go through and update all the documentation, and add more deprecation warnings.

I'm also happy if you want to update the documentation in a follow-up PR.

I am unsure about the components-folder, since they are only used by the TEMController-class, which is now placed in a file outside of the microscope-folder. However, placing controller.py inside the microscope-folder seems unfitting, seeing how important the controller class is.

(TEM)controller.py is meant as a high-level general interface that blends the seperation between the camera and microscope. It makes sense to me to move this to src/instamatic/controller.py. Maybe you could also move ./microscope/components to ./components?

Additionally, should the files not part of the reorganization be left unchanged for now? They will show the deprecation warning, but this reduces the amount of files part of this PR significantly.

As a reviewer, smaller changes are much easier to go through, so feel free to open a new PR with follow-up changes.

Let me know if this PR is finished, I'll be happy to merge it.

src/instamatic/controller.py Outdated Show resolved Hide resolved
@viljarjf viljarjf marked this pull request as ready for review November 7, 2024 13:12
@viljarjf
Copy link
Contributor Author

viljarjf commented Nov 7, 2024

Should be good to go now! I'll fix the deprecation warnings and update the dcumentation in later PRs, then.

Copy link
Member

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, looks good to me!

@stefsmeets stefsmeets merged commit ed2cddd into instamatic-dev:main Nov 7, 2024
7 checks passed
viljarjf added a commit to viljarjf/instamatic that referenced this pull request Nov 8, 2024
stefsmeets pushed a commit that referenced this pull request Nov 11, 2024
* Fix warnings for 3.12

* Fix deprecation warnings introduced in #99
@viljarjf viljarjf mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants