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

Position plan timing resets for each position #187

Closed
wl-stepp opened this issue Sep 19, 2024 · 10 comments · Fixed by #188
Closed

Position plan timing resets for each position #187

wl-stepp opened this issue Sep 19, 2024 · 10 comments · Fixed by #188

Comments

@wl-stepp
Copy link
Contributor

I will open a new issue for this, but it's very similar to #149

On useq-schema-0.4.7:

>>> import useq                                                                                       
>>> seq = useq.MDASequence(stage_positions=[(100,100), (0,0)], time_plan={"interval": 1, "loops": 2}, axis_order='ptgcz')
>>> list(seq)
[MDAEvent(index=mappingproxy({'t': 0, 'p': 0}), min_start_time=0.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 0}), min_start_time=1.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 0, 'p': 1}), min_start_time=0.0, x_pos=0.0, y_pos=0.0), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 1}), min_start_time=1.0, x_pos=0.0, y_pos=0.0)]

See how the min_start_time resets.
With this the first position runs fine, but the later ones will run at full speed.

@wl-stepp
Copy link
Contributor Author

I think the problem with integration in pymmcore-plus might be a bit deeper. For fast acquisitions on larger samples, this could still be a problem even if the counter would not reset. Maybe the timer in pymmcore-plus should actually reset for a new position after the position is set? Might be worth thinking about a little more.

@tlambert03
Copy link
Member

tlambert03 commented Sep 19, 2024

just to confirm, the goal here is to run a complete time lapse at a given position before moving on to the next position right? (I can definitely see how the current min_start_time strategy is insufficient and might require awareness on the acquisition engine as well)

@tlambert03
Copy link
Member

@wl-stepp, on the useq-schema side, what do you think the MDAEvent object should look like in this case? the most straightforward correction here could be to just offset p1 to have a min_start time at least as high as the last event in the previous position:

[MDAEvent(index=mappingproxy({'t': 0, 'p': 0}), min_start_time=0.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 0}), min_start_time=1.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 0, 'p': 1}), min_start_time=1.0, x_pos=0.0, y_pos=0.0), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 1}), min_start_time=2.0, x_pos=0.0, y_pos=0.0)]

That's slightly better, but would still be insufficient in terms of actually yielding the desired result.

We could could also allow an event to "reset" t0:

[MDAEvent(index=mappingproxy({'t': 0, 'p': 0}), min_start_time=0.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 0}), min_start_time=1.0, x_pos=100.0, y_pos=100.0), 
MDAEvent(index=mappingproxy({'t': 0, 'p': 1}), min_start_time=0.0, x_pos=0.0, y_pos=0.0, reset_timer=True), 
MDAEvent(index=mappingproxy({'t': 1, 'p': 1}), min_start_time=1.0, x_pos=0.0, y_pos=0.0)]

other ideas? basically, how do you think this should look on the useq side, such that we can implement the proper behavior on the engine side?

@wl-stepp
Copy link
Contributor Author

just to confirm, the goal here is to run a complete time lapse at a given position before moving on to the next position right? (I can definitely see how the current min_start_time strategy is insufficient and might require awareness on the acquisition engine as well)

Yes.

I think the underlying question might be who should know about some of the timing constraints here, right? I like the reset_timer idea, that way the acquisition engine can also decide when exactly to reset the timer. For a long setup it could reset the timer after it has moved to the new position etc. for example. The user then gives up tight timing from the last event before and the event with the reset_timer flag. I think that's a good option to have for many applications. Our adaptive acquisitions might want to have tight timing up to a trigger but then do something more time consuming and reset the timer with the last event.

Yeah, sounds good to me... How that is reflected in the metadata might be interesting to discuss, but if you give up tight timing across these events you might not be so interested in offsets anyways... And how downstream things might think that the time in the engine is monotonically rising... should there be a timerResetEvent?

@tlambert03
Copy link
Member

tlambert03 commented Sep 21, 2024

as i think about it now, i'm imagining that reset_timer=True is an instruction that all future min_start_times encountered in the sequence should be relative to start time of the the last seen MDAEvent with a timer reset. in terms of implementing it in the engine, I would imagine:

  • there is still going to be a t0 that represents the start of the entire Sequence (i.e. the same timer that currently exists)
  • we can add an additional event-timer that can be reset by the corresponding flag in an MDAEvent. That will be the t0 to which all min_start_time checks are compared.
  • the metadata will always include the full sequence clock (the one that never gets reset)... since one can always back-calculate the effective delay between any acquired images. It may include the event-timer (but I kinda feel like that's unnecessary, since the MDAEvent will also be in the metadata, and that includes the "command" to reset the timer, and actual acquired time is the more important thing. Basically... MDASequence/MDAEvents are what we "want" the engine to do, and the metadata reports what it actually did.

would that all work for you? While we're at it, can you foresee any future needs that this would not meet (such as additional timers, ids, etc...)

@wl-stepp
Copy link
Contributor Author

Yeah, that sounds great.
At the moment I can't think of anything that this concept would not cover. It should be possible to have additional timers if needed inside the adaptive acquisition framework for example, not the engine.

@tlambert03
Copy link
Member

btw, support added in pymmcore-plus in pymmcore-plus/pymmcore-plus#383

@wl-stepp
Copy link
Contributor Author

wl-stepp commented Oct 31, 2024

Hey everyone,

sorry to re-open this, but I came across something that goes wrong here. For subsequences at different positions, somehow all events have the reset_event_timer flag. Looking at the pull request I don't get why...
I have a little more involved sequence in mind, but here is a minimal example:

from pymmcore_plus import CMMCorePlus
from useq import MDASequence
from useq import Position

mmc = CMMCorePlus()
mmc.loadSystemConfiguration()

seq = MDASequence(
    stage_positions=[Position(x=0, y=0, 
                              sequence=MDASequence(channels=["Cy5"],
                              time_plan={"interval": 1, "loops": 5})),
                     Position(x=1, y=1, 
                              sequence=MDASequence(channels=["DAPI"],
                              time_plan={"interval": 1, "loops": 5}))]
)

mmc.run_mda(seq)

Output:

2024-10-31 18:47:23,636 - pymmcore-plus - INFO - (_runner.py:329) MDA Started: stage_positions=(AbsolutePosition(x=0.0, y=0.0, sequence=MDASequence(channels=(Channel(config='Cy5'),), time_plan=TIntervalLoops(interval=datetime.timedelta(seconds=1), loops=5))), AbsolutePosition(x=1.0, y=1.0, sequence=MDASequence(channels=(Channel(config='DAPI'),), time_plan=TIntervalLoops(interval=datetime.timedelta(seconds=1), loops=5))))
2024-10-31 18:47:23,642 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 0, 't': 0, 'c': 0}) channel=Channel(config='Cy5') min_start_time=0.0 x_pos=0.0 y_pos=0.0 reset_event_timer=True
2024-10-31 18:47:24,669 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 0, 't': 1, 'c': 0}) channel=Channel(config='Cy5') min_start_time=1.0 x_pos=0.0 y_pos=0.0 reset_event_timer=True
2024-10-31 18:47:26,685 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 0, 't': 2, 'c': 0}) channel=Channel(config='Cy5') min_start_time=2.0 x_pos=0.0 y_pos=0.0 reset_event_timer=True
2024-10-31 18:47:29,702 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 0, 't': 3, 'c': 0}) channel=Channel(config='Cy5') min_start_time=3.0 x_pos=0.0 y_pos=0.0 reset_event_timer=True
2024-10-31 18:47:33,721 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 0, 't': 4, 'c': 0}) channel=Channel(config='Cy5') min_start_time=4.0 x_pos=0.0 y_pos=0.0 reset_event_timer=True
2024-10-31 18:47:33,736 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 1, 't': 0, 'c': 0}) channel=Channel(config='DAPI') min_start_time=0.0 x_pos=1.0 y_pos=1.0 reset_event_timer=True
2024-10-31 18:47:34,753 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 1, 't': 1, 'c': 0}) channel=Channel(config='DAPI') min_start_time=1.0 x_pos=1.0 y_pos=1.0 reset_event_timer=True
2024-10-31 18:47:36,770 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 1, 't': 2, 'c': 0}) channel=Channel(config='DAPI') min_start_time=2.0 x_pos=1.0 y_pos=1.0 reset_event_timer=True
2024-10-31 18:47:39,787 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 1, 't': 3, 'c': 0}) channel=Channel(config='DAPI') min_start_time=3.0 x_pos=1.0 y_pos=1.0 reset_event_timer=True
2024-10-31 18:47:43,804 - pymmcore-plus - INFO - (_runner.py:290) index=mappingproxy({'p': 1, 't': 4, 'c': 0}) channel=Channel(config='DAPI') min_start_time=4.0 x_pos=1.0 y_pos=1.0 reset_event_timer=True
2024-10-31 18:47:43,819 - pymmcore-plus - INFO - (_runner.py:416) MDA Finished: stage_positions=(AbsolutePosition(x=0.0, y=0.0, sequence=MDASequence(channels=(Channel(config='Cy5'),), time_plan=TIntervalLoops(interval=datetime.timedelta(seconds=1), loops=5))), AbsolutePosition(x=1.0, y=1.0, sequence=MDASequence(channels=(Channel(config='DAPI'),), time_plan=TIntervalLoops(interval=datetime.timedelta(seconds=1), loops=5))))

The t index is not zero, so I don't get why they would get the flag...

@wl-stepp
Copy link
Contributor Author

wl-stepp commented Oct 31, 2024

Is this something that might be handled by #192 ? @tlambert03

@tlambert03
Copy link
Member

not #192, but I liked and merged your fix in #194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants