Migrate acquisition engine to pymmcore-plus #177
Conversation
…on steps fail, cleanup_daq() is called, which then throws another exception if these aren't initialized.
…id in pymmcore-plus
…denoting other cleanup that needs to be done)
… misspelled anyway)
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the acquisition engine from pycromanager to pymmcore-plus, updating API method names and associated acquisition event handling throughout the codebase.
- Added the pymmcore_plus dependency in pyproject.toml.
- Replaced legacy pycromanager calls with updated camelCase methods from pymmcore_plus.
- Adjusted acquisition event generation and updated various configuration and device property calls.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added dependency "pymmcore_plus==0.13.6". |
| mantis/cli/run_acquisition.py | Simplified demo run detection by using a case-insensitive match. |
| mantis/acquisition/microscope_operations.py | Updated method calls to follow the new pymmcore_plus camelCase API. |
| mantis/acquisition/acq_engine.py | Migrated acquisition engine implementation to use pymmcore_plus; updated event generation and device calls. |
| examples/acquisition_settings/demo_acquisition_settings.yaml | Updated camera BitDepth value reflecting new acquisition requirements. |
Status 05/12/25TLDR
Detail As has been noted before, I'm trying to make the port as minimally invasive as I can. Thus, where possible, have just tried to use analogous data structures between pycromanager & pymmcore_plus. The main data structure conversions are:
The above handled most of the engine but there were a couple of modifications that needed to be made: both related to the looping structure, where the outer time & position loops are handled by ShrimPy, while the inner channel & z loops are handled by the engine, which allows autofocus and autoexposure to happen at each new position. Previously, a copy of the acquisition_event instance was made and the copy was then modified to have the correct indices. That couldn't be exactly replicated using pymmcore-plus because MDAEvent inherits from a read-only class. To get around this, I took advantage of the fact that one can use a list of MDA Events as if it were a MDASequence, so I simply regenerate the sequence by iterating over it, and appending a new object to a list created with modified constructor args. |
jeskesen
force-pushed
the
170-migrate-to-pymmcore-plus
branch
from
59f95d2 to
28a023a
Compare
| if self.ls_acq.enabled and self._ls_z_ctr_task is not None: | ||
| self._ls_z_ctr_task.stop() | ||
| self._ls_z_ctr_task.close() | ||
|
|
||
| if self.lf_acq.enabled: | ||
| self._lf_z_ctr_task.stop() | ||
| self._lf_z_ctr_task.close() | ||
| self._lf_channel_ctr_task.stop() | ||
| self._lf_channel_ctr_task.close() | ||
| if self._lf_channel_ctr_task is not None: | ||
| self._lf_channel_ctr_task.stop() | ||
| self._lf_channel_ctr_task.close() | ||
| if self._lf_z_ctr_task is not None: | ||
| self._lf_z_ctr_task.stop() | ||
| self._lf_z_ctr_task.close() | ||
|
|
There was a problem hiding this comment.
I added the following checks because this code can get called as part of exception handling at startup before these these data structures are created.
| config_group : str, optional | ||
| config_name : str, optional | ||
| use_pycromanager : bool, optional | ||
| use_pymmcore_plus : bool, optional |
There was a problem hiding this comment.
@ieivanov, do we still need this boolean?
There was a problem hiding this comment.
This function had two ways to acquire data - (1) using direct calls to mmc.snapImage() via microscope_operations.acquire_defocus_stack and (2) using the pycromanager interface. Using pycromanager added the benefit of doing sequences (aka hardware triggered) Z stack acquisitions in which the camera is set to acquire a set of frames at its max framerate and the Z stage is stepped on every exposure pulse. microscope_operations.acquire_defocus_stack on the other hand allows data to be displayed in a MicroManager window, but we don't use that functionality within acquire_ls_defocus_stack. All in all, I think we can just switch to using pymmcore-plus acquisitions that store the data in the data array (note that here we return the data structure as numoy array). The sequences vs not sequenced acquisition is controlled externally by enabling sequencing within the Z stage properties.
mantis/acquisition/acq_engine.py
Outdated
| reset_event_timer=event.reset_event_timer | ||
| ) | ||
|
|
||
| lf_events = [mda_event_from_mda_sequence(event) for event in lf_cz_events] |
There was a problem hiding this comment.
Previously, deep copies were made of the events (why?), and then those were modified in place. Since an MDAEvent is immutable, my workaround was to create a list of new objects with slightly modified constructor args.
A better way of doing this would be to just create the MDASequence here, inside the loop... but I wanted to touch the program flow as little as possible for now.
There was a problem hiding this comment.
No need to try to work too hard to maintain the original program flow if it doesn't make sense. I think it's OK to create the correct MDAEvent on every iteration of the loop, rather than creating a set of them at the beginning and modifying them as we go.
If it makes the built-in writers work better, we could also create a sequence of events for all timepoint and positions at the start, and then skip some of them if, for example, O1 autofocus fail. You're welcome to get creative where it makes sense rather than trying to stick too close to our old workflows if that's cumbersome.
| self._lf_acq_obj.mark_finished() | ||
|
|
||
| # JGE: HACK to indicate finished. | ||
| # self._ls_acq_obj.mark_finished() |
There was a problem hiding this comment.
Commented out during development, needs to be put back in.
See #180
* reformatted using black * run on CI on pymmcore-plus branch (revert me before merge to main) * matched version of black & python to the one used CI, and ran it again * addressed flake8 complaints * isort fixes * increase verbosity of pytest for debugging on windows (without a windows machine to debug on) * address github copilot comments * crank the verbosity to 11! * typo * one last stab in the dark as to what the failure in windows might be before I actually set myself up a windows machine on which to debug this. This is just a revert of this particular line its current state on main.
|
Note: pymmcore-plus uses a default PYMM_BUFFER_SIZE_MB of 250 MB which is quite low. When using MicroManager we set that to ~5 GB, I'll check and we should set that at startup. |
* initial MM generated config (for Windows) * Added slightly modified copy/paste of windows config for linux * Modify name of Oryx camera to "Oryx" * WIP: allowing lightsheet to be disabled * Revert "WIP: allowing lightsheet to be disabled" This reverts commit 1748131. * LF can run without LS * style fixes * small bugfixes * get correct ROI size for zarr output * for zarr output, remove channel dimension if <= 1 * fix Oryx camera ID in getProperty query * ran black * added checks to verify that acq engine is enabled * Ivan's changes to the example file for on-device development * Temporary edits to enable LF only * On device hacks for debugging. revert later * MDA really only handles channel and slice. Remove position & time setting when copying the events * Allow label-free config to be set on the command line. ran black. * Calculate the slice acquisition rate for LF even if LS is disabled. Access Oryx camera using mmc.getCameraDevice() instead of hard-coded string * Fix logic in code which checks for light-sheet completion and aborts when enabled. * Uncomment out acquisition & processing that happens inside the position loop * removed squid configs... they should be in the configs repo * Added DAQ sequencing back to acquisition. Frames are now being acquired * fixed callback which starts the DAQ for the sequenced acquisition * downgraded black to 22.3.0 on mantis (pymmcoreplus conda env), and re-rean * Ran flake8 * fixed abort-check logic * changed mm_config to ls_config * reverting example_acquisition_settings to pre-branch state --------- Co-authored-by: ieivanov <tayllamth@gmail.com>
* fixed abort-check logic (again). Fixed indexing bug in LS daq counter init * fixed a stray mm-config-filepath that didn't get updated previously * update autofocus calls to use the core rather than Studio * in autofocus, get position from the proper device. * style fixes * updating autofocus to respond to Ivan's feedback
* Update author metadata labeling in CITATION.cff (#188) Update author metadata format in CITATION.cff * Add CLI to stir plate before acquisition (#189) * add stir_plate CLI * docs and check for empty pos list * use logging instead of click.echo * style * Autoexposure safety checks (#190) * check min exp time * check for demo mode after validating illumination.csv file * bugfixes and extensions * bugfix * style --------- Co-authored-by: Mandy Chen <34908590+picaq@users.noreply.github.com> Co-authored-by: Ivan Ivanov <ivan.ivanov@czbiohub.org>
* in acquisition abort check, change check from mmc.isSequenceRunning to mmc.mda.is_running. this is more general * handle exception when failing to stop a sequenced stage by printing warning rather than crashing. * update hook LS hook function calls for pymmcore-plus * style fixes * upload daq counter tasks as a single call, instead of 1-per microsope instance * Added start to daq counter for lightsheet which fixed a problem the acquisitions weren't getting triggered for channels > 1.
* updated acquire-zarr to 0.6.0 * Added configuration class for zarr output * update example configs with new settings * made zarr chunk sizes into a dictionary * make shard sizes properly configurable * added initial HCS support * ran black * removed chunking or sharding in time / position dimensions * ran black * remove performance warnings, because I lack data on where any performance cutoff is * ran isort * per @ievanov's suggestion, broke out zarr init into its own function * added Ivan's suggestions for well name defaults
* Update author metadata labeling in CITATION.cff (#188) Update author metadata format in CITATION.cff * Add CLI to stir plate before acquisition (#189) * add stir_plate CLI * docs and check for empty pos list * use logging instead of click.echo * style * Autoexposure safety checks (#190) * check min exp time * check for demo mode after validating illumination.csv file * bugfixes and extensions * bugfix * style * Abort stalled o3 autofocus (#203) * abort stalled O3 autofocus * add note on hardcoded value * O3 autofocus improvements (#204) * add extra logging and make sure O3 will not run outside of travel range * fix bug in galvo positioning during o3 refocus * set o3 stage position at beginning of new acquisition * merge implementation * enable LS by default * uncomment and connect update_ls_hardware * modified update_ls_hardware to get the channel index from an MDAEvent * ran black * added channel list back into args of update_ls_hardware * update_ls_hardware finally works * appeasing the style gods * yet more appeasing the style gods, this time isort --------- Co-authored-by: Mandy Chen <34908590+picaq@users.noreply.github.com> Co-authored-by: Ivan Ivanov <ivan.ivanov@czbiohub.org>
* remove position/time chunking/chunking from examples yamls * fix previously ommitted zarr-config reading in cli code * how am I still hitting camel-case issues? * fix plate argument * fix zarr store initialzation so that position_settings can be accesed * pass output_path directly to initialize_zarr_store * fix incorrect StreamSettings constructor arg * fix zarr compression configuration * add .zarr suffix to output file if one does not exist
This PR addresses #170 .
This is a copy of #171, but we've moved the development branch into this repository rather than a fork in the acquire-project.
Since migrating the acquisition engine is largely an all-or-nothing endeavor, this PR tracks the entire project's progress: whereas individual features will be merged into this branch with their own PRs.