-
Notifications
You must be signed in to change notification settings - Fork 31
Simulation with dependencies #140
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
base: main
Are you sure you want to change the base?
Simulation with dependencies #140
Conversation
@Baharis This is what I had that we talked about last week. I want to look though and check this, but I might not have time for a few weeks (and at that point I might have even less time...). I believe I tried the eucentric height finder script, and it worked. This (and similar scripts/functions) is why I wanted to have a more accurate simulation in the first place. Simulating the actual microscope layout is very useful for testing basic script functionality without having very expensive microscopes doing very simple stuff. |
@viljarjf Thanks for looking back at this! Au contraire, I am extremely busy this week but I will be sure to look at, test, adapt if needed, and suggest a way to include it in Instamatic ASAP as I believe this is too big inclusion to sleep on. I will be in touch once I can! |
I finally had some time to analyze #105 and #140. This "integrated" simulator is great, and I am deeply impressed how fast and seamless it is. I truly believe this is great work, @viljarjf! However, I am not a fan of how deeply does this implementation ingrain simulation in Instamatic. It is not only the matter of additional dependencies: #140 in particular makes this integrated simulator a special and core part of Instamatic. The new camera simulator replaces the current one completely (so the dependency is not optional). Simulated cam and tem can't be run independently anymore, something I was doing routinely for my testing. I summarized my opinions and categorized all currently suggested and already introduced changes into four categories, as differentiated by their prefix emojis:
The four categories are as follows:
Concerning the 🔦 simulation code**, I read through #104, #105, #140 and personally favor @stefsmeets 's "optional dependency" strategy more; that is not to say I like it. Thinking about existing instances of standalone instamatic code e.g. instamatic-tecnai-server I thought to suggest a completely different "standalone" solution: putting all simulation code into a separate git repository e.g.
@viljarjf This may be quite some work and I know you have little time now. I could try to squeeze it in my schedule, but I would love to hear y'all opinion first (also @stefsmeets ). |
Thank you for your extensive comments @Baharis! I'm sorry this mess of a branch needed categorization of feedback, it really is just a jumbled collection that also include the simulations. A lot of this was done months ago, where random lines here and there broke some button or feature in the gui, which I seemingly commited before sharing the branch here. I also, as you say, completely replaced the existing simulation camera, as it was easier when I was just playing around. I never got around to properly writing it as a dependency, with import checking ect. I can spend some time later moving the simulations to a seperate repo, and see how it turns out. It would also allow for seperate/more expansive config files to allow user-specified parameters for crystal structures, dispersion, and grid parameters ect. As you said, a lot of the things here is full of TODOs, and in a seperate repo it's easier to keep track of these things with issues ect without spamming down the main repo here. The already merged implementation deserves some touching up too. It could in principle be compressed into a single class, and the unit cell handling prorvided by diffpy could be manually re-implemented without that much effort to remove the dependency altogether. The current implementation, with an entire submodule with seperate files and classes for everything, was written with expandability in mind, and would indeed fit better as a seperate project. So, I propose the following:
As for time, I think the first point above is most pressing and straightforward for me to do. I'll squeeze it in next week, hopefully. The second point requires more work, so it might be a weekend project later rather than something I make time for in my last two weeks now. In Stef's original mock code he called it DigiTEM, maybe that could be the project name? |
@viljarjf I think that the scope of the refactor may be smaller than we think. A lot of files need moving, but all the functionality is already implemented, so it is a matter of putting the jigsaw pieces back together. As for "who", I would love to help but this is your project to begin with so I do not want to take your agency. As for "where", I'd definitely start the name with Personally, I never got the simulated image/diffraction to work on my setup – I never found out how. I may be biased, however, what would you say to moving all the simulated code to the separate repo and leaving only the old, most bare-bone simulation code in? This remove dependency and leaves the code in its simplest and fastest version, which I believe is its goal to begin with. |
Thanks for the nice analysis @Baharis
This was also the solution that popped into my head. A simulator like this by necessity needs to tightly couple the tem and camera bits and manage state for longer period of time. By splitting it out into a seperate process you can do whatever is necessary to create the images and manage state. Instamatic just sees the images and tem state over the network. This will also make it easier to update in the long run (less risk of breakage), scope can be enormous and the dependencies as heavy as needed. For me the key is that the core instamatic part (controller, tem/cam interface) remain simple to use. There are already quite a few parts that probably nobody would ever use anymore. I think at some point I wanted to just have a light package for microscope control, but that never worked out 😅
Sounds like a workable plan to me. Feel free to use the |
FYI, I have 8 hours of undisturbed quality work time today, so unless you object @viljarjf I will try moving your implementation to a separate project. Contact me at tchon-at-fzu-dot-cz if needed. |
Here is an update on my work moving this branch to a separate file. TLDR: surprisingly easy with odd caveats. Refactoring journal![]() I made a new repository https://github.com/Baharis/instamatic-TEM-emulator for testing. First, I wrote an tem = EmulatedDeviceKind('microscope', SimuMicroscope, logging.getLogger('tem'))
cam = EmulatedDeviceKind('camera', CameraEmulator, logging.getLogger('cam'))
tem_server = EmulatedDeviceServer(device_kind=tem)
tem_server.start()
cam_server = EmulatedDeviceServer(device_kind=cam, tem=tem_server.device)
cam_server.start()
signal.signal(signal.SIGINT, handle_keyboard_interrupt)
threading.Thread(target=listen_on, args=(5000, tem)).start()
threading.Thread(target=listen_on, args=(5001, cam)).start() The communication part works now without issues. I needed to make a tiny but important change to Instamatic – I changed only 2 (!) lines to make the server camera streamable. For some reason, Instamatic enforces cameras ![]() My biggest problem is that for the love of God I cannot get the diffraction simulation to work. I moved the simulation files (other than microscope) to the new repo, but it seems the diffsims interface is rather unstable – to the degree, where the code does not want to work with either version 0.6.1 or 0.7.0 (released this summer), despite my attempts to adapt it. Consequently, the direct images are streamed and shown fine, but for the time being switching to diffraction kills it. How to run the emulatorThis will make its way to the README at one point, but for the time being: to run the emulator, one needs to:
|
I sat down to look at it, and I have used features which I wrote in pyxem/diffsims#232 in the sim rework. This was less than obvious, I'm sorry for that. I have worked on finalizing that PR, and had some upstream hang-ups in orix as well. I'll try to get them fixed up, for now maybe we can use the old sim-code that worked (so before this PR)? |
@viljarjf I have thoroughly tested the server part at this point, so I intend to publish some version of this emulator to a new repo in instamatic today and an associated instamatic patch PR later. Granted I believe it would take you much less time than me to adapt it, would you mind pushing a patch there then? I did not touch the simulation part at all, only made imports relative for development and used the instamatic simulated microscope rather than real one. No need for PR. Also, I called it |
@Baharis Sounds great, I can fix it up once you publish it. I don't feel strongly about the name. |
I published the new repository as https://github.com/instamatic-dev/instamatic-TEM-emulator. I added a detailed README, but one thing that is not stressed enough there is that currently to run the emulator, one needs to apply the following patch: Subject: [PATCH] Remove unnecessary debug line
---
Index: src/instamatic/camera/camera_client.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/instamatic/camera/camera_client.py b/src/instamatic/camera/camera_client.py
--- a/src/instamatic/camera/camera_client.py (revision 61a3bdb00d2bcc562ec52664fbf01a8cf6a3d456)
+++ b/src/instamatic/camera/camera_client.py (date 1758814315337)
@@ -55,7 +55,7 @@
self.name = name
self.interface = interface
self._bufsize = BUFSIZE
- self.streamable = False # overrides cam settings
+ # self.streamable = False # overrides cam settings
self.verbose = False
try:
Index: src/instamatic/camera/camera.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/instamatic/camera/camera.py b/src/instamatic/camera/camera.py
--- a/src/instamatic/camera/camera.py (revision 61a3bdb00d2bcc562ec52664fbf01a8cf6a3d456)
+++ b/src/instamatic/camera/camera.py (date 1758814315337)
@@ -56,7 +56,7 @@
from instamatic.camera.camera_client import CamClient
cam = CamClient(name=name, interface=interface)
- as_stream = False # precaution
+ # as_stream = False # precaution
else:
cam_cls = get_cam(interface) This patch allows server cameras to be streamable, a feature disabled by default by @stefsmeets that actually works quite fine on local server. A version of this patch with my exasperated comments is currently available at https://github.com/Baharis/instamatic/tree/instamatic-TEM-emulator. I plan to make a new PR that includes these changes alongside some camera generalizations. For the time being, I would appreciate your contribution there @viljarjf. Upon implementation I believe this will close #140 and #105; as for #141, I am still of opinion that the |
Unfinished, but this intends to improve the simulations to use not only positions, but also intensities of reflections.
Here is a preview:
2025-09-04.11-25-28.mp4
Some changes in diffsims, the simulation library I used, might let this be even simpler and faster. A lot of the diffraction image preparation is copied from there.