-
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 |
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.