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

feat: Live mode button widget #148

Merged
merged 33 commits into from
Jun 9, 2022
Merged

Conversation

fdrgsp
Copy link
Contributor

@fdrgsp fdrgsp commented Mar 11, 2022

Creating a new general purpose two-state (on-off) Live mode QPushButton widget.

It works with new startContinuousSequenceAcquisition and stopSequenceAcquisition events signals added to pymmcore-plus in PR https://github.com/tlambert03/pymmcore-plus/pull/115.

When the button is toggled on, a continuous Sequence Acquisition is started (using a new pymmcore-plus startContinuous Sequence Acquisition method) and a starting signal is emitted. In main_window the signal is connected to the _toggle_live function where the new frames are added to the napari viewer through a QTimer.

When the button is toggled off, the Sequence Acquisition is stopped (using a new pymmcore-plus stop Sequence Acquisition method). The connected 'stop' signal connected to main_window will stop updating the napari viewer and resets the QTimer.

@ianhi
Copy link
Contributor

ianhi commented Mar 11, 2022

When toggled on, a signal is emitted (_emitFrameSignal) through a QTimer. In main_window the signal is connected to the update_viewer function.

@tlambert03 @ianhi Do you think it can be a valid solution?

This is a really good start! But I think we can go farther in the decoupling if we can stomach adding a new core signal. I think my ideal would be:

  1. core gets the new signals: continuousSequenceAcquisition[Started/Stopped]
  2. Live button is only responsible for:
    • stopping and starting the sequence
    • Temporarily pausing the acquisition when the exposure is updated
      (actually this could maybe even be moved onto core)
  3. main_window listens to the new signals and starts its own QTimer based on the exposure

This works out if all listeners call getLastImage because that doesn't actually remove anything from the circular buffer.

I think that that's more or less analogous to what your doing with the snap button. This is also y preference because it feels like a cleaner separation of concerns. The live button is just responsible for toggling a single state on core - not managing timers and emitting data. Ideally the only place other components receive data from is the core.

Alternatively, we can push the timer logic into the button as you've done now. But in that case I think we should:

  1. make the signal public
  2. consider changing the name - in this case it's really more of a live_manager than a button.

Co-authored-by: Ian Hunt-Isaak <ianhuntisaak@gmail.com>
@fdrgsp fdrgsp marked this pull request as draft March 11, 2022 20:01
@fdrgsp fdrgsp changed the title Live mode button widget [WIP] Live mode button widget Mar 11, 2022
@fdrgsp
Copy link
Contributor Author

fdrgsp commented Mar 19, 2022

some tests are failing because of the new signals not yet added to pymmcore-plus.

@fdrgsp fdrgsp changed the title [WIP] Live mode button widget Live mode button widget Apr 15, 2022
@fdrgsp fdrgsp marked this pull request as ready for review April 15, 2022 19:03
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #148 (d7691a1) into main (2384b6d) will increase coverage by 0.54%.
The diff coverage is 85.52%.

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   82.68%   83.23%   +0.54%     
==========================================
  Files          27       28       +1     
  Lines        2385     2433      +48     
==========================================
+ Hits         1972     2025      +53     
+ Misses        413      408       -5     
Impacted Files Coverage Δ
micromanager_gui/main_window.py 77.41% <71.42%> (+4.13%) ⬆️
...romanager_gui/_core_widgets/_live_button_widget.py 86.56% <86.56%> (ø)
micromanager_gui/_gui_objects/_tab_widget.py 93.42% <100.00%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2384b6d...d7691a1. Read the comment docs.

# test from direct mmcore signals
with qtbot.waitSignal(global_mmcore.events.startContinuousSequenceAcquisition):
global_mmcore.startContinuousSequenceAcquisition(0)
assert live_btn.text() == "Stop"
Copy link
Member

@tlambert03 tlambert03 Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you want this assert (and those on ln 34,35,40,41,45,46) to be outside of the waitSignal context. Though it may work as is it's likely to be just because it happens nearly instantaneously. but if the goal is to make the wait for the signal occur before you make the assertions, then the assertions should be after the context.

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, couple minor comments on the tests. @ianhi, I think this generally follows the pattern you were also endorsing. Good with this?

@tlambert03 tlambert03 changed the title Live mode button widget feat: Live mode button widget Jun 9, 2022
@tlambert03 tlambert03 merged commit f0893a7 into pymmcore-plus:main Jun 9, 2022
@ianhi
Copy link
Contributor

ianhi commented Jun 9, 2022

This looks good to me, couple minor comments on the tests. @ianhi, I think this generally follows the pattern you were also endorsing. Good with this?

missed this but yes it does! nice stuff

@fdrgsp fdrgsp deleted the live_mode_button branch May 27, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants