Skip to content

Conversation

viljarjf
Copy link
Contributor

@viljarjf viljarjf commented Sep 4, 2025

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.

@viljarjf
Copy link
Contributor Author

viljarjf commented Sep 4, 2025

@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...).
If you want to adapt this to something distributable that is totally fine. Otherwise, the simulations already available should at least be a decent start for incoming users, they should at least be consistent with the unit cell.

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.

@Baharis
Copy link
Member

Baharis commented Sep 9, 2025

@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!

@Baharis Baharis added enhancement dependencies Pull requests that update a dependency file labels Sep 9, 2025
@Baharis Baharis linked an issue Sep 18, 2025 that may be closed by this pull request
@Baharis
Copy link
Member

Baharis commented Sep 18, 2025

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:

  • 🔦 pyproject.toml – adds 3 dependencies necessary for image simulation
  • 🐛 src/instamatic/calibrate/calibrate_beamshift.py: bugfix (that I don’t understand)
  • 🐛 src/instamatic/camera/fakevideostream.py: bugfix (that I don’t understand)
  • 🐛 src/instamatic/controller.py: some potential bugfixes (that I don’t understand)
  • 🎁 src/instamatic/controller.py: adds useful but unused get_rotated_raw_image
  • 🐛 src/instamatic/experiments/autocred/experiment.py: minor unrelated (?) changes
  • 🐛 src/instamatic/experiments/red/experiment.py: minor unrelated (?) changes
  • 🐛 src/instamatic/gui/autocred_frame.py: minor unrelated (?) changes
  • 🎁 src/instamatic/gui/ctrl_frame.py: adds a few new cool features to the hand panel
  • 🔦 src/instamatic/microscope/microscope.py: default tem/cam sim → the new one
  • 🐛 src/instamatic/processing/find_crystals.py: minor bugfix I guess?
  • ❌ src/instamatic/simulation/init.py – return joined cam/sim if either requested
  • 🔦 src/instamatic/simulation/camera.py – simple internal camera client?
  • 🔦 src/instamatic/simulation/crystal.py – simulates diffraction using dependencies?
  • 🔦 src/instamatic/simulation/grid.py – 130 lines of grid simulation, mostly TODOs
  • 🔦 src/instamatic/simulation/microscope.py – moved here from simu microscope
  • 🔦 src/instamatic/simulation/sample.py – 81 lines of sample collision checks
  • 🔦 src/instamatic/simulation/stage.py – simulates diffraction using dependencies?
  • 🎁 src/instamatic/simulation/warnings.py – neat NotImplementedWarning only
  • 🔦 tests/test_simulation/init.py – empty file
  • 🔦 tests/test_simulation/test_crystal.py – simple test of a cubic lattice nodes?
  • 🔦 tests/test_simulation/test_grid.py – 2 simple and 2 xfail grid “tests”
  • 🔦 tests/test_simulation/test_sample.py – 3 simple and 2 xfail sample “tests”
  • 🔦 tests/test_simulation/test_stage.py – 2 simple and 2 xfail grid “tests”

The four categories are as follows:

  • 🔦 these are all core elements of the simulation code, and I will comment on these further below;
  • 🐛 these seem to be bugfixes unrelated to other categories found when adding simulation code
  • 🎁 these are neat standalone features that would be great in Instamatic but are unrelated to simulation
  • ❌ this one marks the get_simulation_instances that IMO in the current shape is problematic

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. instamatic-virtual (as name "simulated" is taken 😛) and running it as a standalone program in its separate process. Here is how would this work:

  • Standalone simulator is given a new git and pip project; ideally it has its own small sets of tests and CI; it uses instamatic and all other necessary libraries (diffpy, diffsims, orix) as dependencies.
  • The user chooses to install this simulator in instamatic environment if they care and can afford the simulation. They can connect to and run the standalone simulator by providing its local IP in settings (may be fixed, or maybe even auto-detected).
  • Internally, standalone simulator uses default instamatic tem simulator, @viljarjf's src/instamatic/simulation/ camera (absent on main branch) to generate images; it runs on the instamatic PC in a seperate process using separate threads for sim and cam.
  • instamatic runs in a different process than the simulator so it does not affect the simulation time this much, however, serialization is now needed to pass the images. This may add small (ms) consistent lag, similar to real cameras.
  • If anyones wants to have a better simulator, they can run it on a stronger computer, connect via remote IP, and fork the standalone simulator repository instead of the entire instamatic; this is much more DRY and does not cut them from main branch development; in long term this increases maintainability and the chance other developers contribute their code to main.

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

@viljarjf
Copy link
Contributor Author

Thank you for your extensive comments @Baharis!
I fully agree with the idea of seperating this into its own repo. This principle can (and maybe should) be applied to other parts of instamatic as well.

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:

  • refactoring the current camera simulation to remove all TODOs, unnecessary features, and dependencies, and condense it into a single class/file
  • Create a seperate repo for simulating the TEM. The scope of this can easily be enormous, but kinematical simulation of dispersed, round, perfect crystals on a grid is a good start. Eventually, we could for example add simple optics (Andrew Stewart's group has some tools for this with TEMGYM) to calculate the illuminated area on the grid.
  • Cherry-pick the few 🎁s from this branch

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?

@Baharis
Copy link
Member

Baharis commented Sep 19, 2025

@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 instamatic. instamatic-digitem is fine with me. I can create a new repo in instamatic-dev, preferably after getting a green light from @stefsmeets.

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.

@stefsmeets
Copy link
Member

stefsmeets commented Sep 19, 2025

Thanks for the nice analysis @Baharis

  • The user chooses to install this simulator in instamatic environment if they care and can afford the simulation. They can connect to and run the standalone simulator by providing its local IP in settings (may be fixed, or maybe even auto-detected).

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 😅

  • refactoring the current camera simulation to remove all TODOs, unnecessary features, and dependencies, and condense it into a single class/file

  • Create a seperate repo for simulating the TEM. The scope of this can easily be enormous, but kinematical simulation of dispersed, round, perfect crystals on a grid is a good start. Eventually, we could for example add simple optics (Andrew Stewart's group has some tools for this with TEMGYM) to calculate the illuminated area on the grid.

  • Cherry-pick the few 🎁s from this branch

Sounds like a workable plan to me. Feel free to use the instamatic-dev organization for this as @Baharis suggested (but also feel free to host in your own repo instead). Let me know if I need to sort out any access rights on Github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants