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

Cross-figure event passing system #11685

Merged
merged 59 commits into from
Aug 16, 2023
Merged

Cross-figure event passing system #11685

merged 59 commits into from
Aug 16, 2023

Conversation

wmvanvliet
Copy link
Contributor

@wmvanvliet wmvanvliet commented May 10, 2023

Proposed in #11678, this PR works on the actual implementation of mne.viz.ui_events. The idea is to run our own event passing system so that figures can talk to each other. For example, if you change the time slider on one figure, the time in other figures is updated accordingly.

TODO:

  • the core of the event system:
    • publish
    • subscribe
    • unsubscribe
    • link,
    • unlink
    • disable_ui_events context manager
  • implementation of the TimeChange event for plot_topomap(time='interactive')
  • make all unit tests pass again
  • example of linking topomap plots and a custom plot
  • (developer) documentation of the ui-event system
  • add UI-Events section in the API docs for figures implementing ui-events
  • update What's new
  • Move to new folder tutorials/visualization
  • Move publication figure example to tutorials/visualization with a redirect (in doc/conf.py)
  • wait for 1.5 release

Follow-up PRs:

Closes #11678

@wmvanvliet
Copy link
Contributor Author

wmvanvliet commented May 10, 2023

@larsoner is right, we need a better name than just "event" because that is easily confused with MEG event markers.

edit: this change has now been made.

@larsoner
Copy link
Member

I think I prefer UIEvent. Another option would be FigEvent

@hoechenberger
Copy link
Member

What is the current status / plan here?

@wmvanvliet
Copy link
Contributor Author

Status is that the basic ui-event system seems to be working nicely. However, there are unit tests failing still (that somehow worked when testing locally?).

The plan for this PR is to only implement the ui-event system and the time_change event for Brain and plot_topomap, so it is manageable to review. Then in subsequent PRs we can start implementing ui-events for more of our plots. I've started a TODO section in the PR description up top.

@wmvanvliet
Copy link
Contributor Author

In the upcoming developer sprint, the ui-event system can be a nice thing to work on.

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@wmvanvliet
Copy link
Contributor Author

Leaving this for 1.6 sounds good. This PR only makes sense in combination with a bunch of others that integrate the UI events into the interactive figures.

@larsoner
Copy link
Member

@wmvanvliet FYI I edited the top comment with a tentative plan, feel free to adjust as you see fit

@larsoner larsoner added this to the 1.6 milestone Aug 14, 2023
@wmvanvliet wmvanvliet self-assigned this Aug 15, 2023
@wmvanvliet wmvanvliet changed the title [MRG] Cross-figure event passing system Cross-figure event passing system Aug 16, 2023
@wmvanvliet wmvanvliet marked this pull request as draft August 16, 2023 07:40
@wmvanvliet
Copy link
Contributor Author

Working on Brain overhaul brought up a new issue with UI events that needs implementing: a way to temporarily disable them.

An issue with the design of the UI event system is how it interacts with GUI elements:

  1. figure receives UI event requesting new value
  2. figure updates plot and GUI elements to match new value
  3. GUI element's set.value() call causes new UI event to fire
  4. infinite loop :(

I thought the loop could be prevented by basically adding boilerplate to the event handlers checking whether new_value == old_value and silently dropping the event if that was the case. However, now I come across GUI elements that subtly change the value. For example:

  1. figure receives UI event requesting a new value of 1.3453
  2. figure updates plot and GUI text box that rounds the value to 1.35
  3. text box value set_value() causes new UI event to fire with value of 1.35
  4. figure receives UI event requesting a new value of 1.35
  5. figure updates plot and GUI text box to 1.35 :(

I guess a more elegant solution is a context manager disable_ui_events that will temporarily silence the production of UI events. An event handler can wrap its GUI updating code with that to prevent loops.

@mscheltienne
Copy link
Member

A signal blocker, disable_ui_events, similar to QSignalBlocker in Qt seems appropriate.

@wmvanvliet wmvanvliet marked this pull request as ready for review August 16, 2023 09:20
@larsoner
Copy link
Member

Good to go again @wmvanvliet ? We cut 1.5 yesterday so this can be merged after one final review I think

Comment on lines 70 to 75
def callback(event):
"""Respond to time change event."""
global callback_called
callback_called = True
assert isinstance(event, ui_events.TimeChange)
assert event.time == 10.2
Copy link
Member

Choose a reason for hiding this comment

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

FYI to avoid this global business another (quicker/easier usually) approach is to use something mutable like a list:

callback_calls = list()

def callback(event):
    callback_calls.append(event)

...
assert len(callback_calls)  # or whatever

It automatically deals with this scoping stuff. I'll push a commit to do it then mark for merge when green, but if you really like the global approach I can live with it (and you can put it back in the next PR)!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong feelings about it :)


Bugs
~~~~
- None yet

API changes
~~~~~~~~~~~
- None yet
- Added :mod:`mne.viz.ui_events` API for controlling interactive figures (:gh:`11685` by `Marijn van Vliet`_).
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really an API change the way we use it in latest.inc... it's a feature. API is really for deprecations

@larsoner larsoner enabled auto-merge (squash) August 16, 2023 16:39
@larsoner larsoner merged commit 0e68aba into mne-tools:main Aug 16, 2023
26 checks passed
larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 16, 2023
* upstream/main:
  Refactor test_epochs.py::test_split_saving (2 out of 2) (mne-tools#11884)
  Cross-figure event passing system (mne-tools#11685)
@wmvanvliet
Copy link
Contributor Author

Cool! Now we can build lots of PRs on top of this. Adding interactivity everywhere.

@wmvanvliet wmvanvliet deleted the events branch August 17, 2023 11:13
@hoechenberger
Copy link
Member

Great!

larsoner added a commit to drammock/mne-python that referenced this pull request Aug 22, 2023
* upstream/main:
  [pre-commit.ci] pre-commit autoupdate (mne-tools#11911)
  [BUG, MRG] Remove check on `mne.viz.Brain.add_volume_labels` (mne-tools#11889)
  Small splits fix (mne-tools#11905)
  adds niseq package to "Related software" (mne-tools#11909)
  Minor fixes for ERDS maps example (mne-tools#11904)
  FIX: Fix pyvista rendering (mne-tools#11896)
  BUG: Fix epoch splits naming (mne-tools#11876)
  ENH: Use section-title for HTML anchors in Report (mne-tools#11890)
  CI: Deploy [circle deploy]
  MAINT: Clean up whats_new and doc versions (mne-tools#11888)
  Refactor test_epochs.py::test_split_saving (2 out of 2) (mne-tools#11884)
  Cross-figure event passing system (mne-tools#11685)
  MAINT: Post-release deprecations, updates [circle deploy] (mne-tools#11887)
  MAINT: Release 1.5.0 (mne-tools#11886)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#11883)
  Refactor test_epochs.py::test_split_saving (1 out of 2) (mne-tools#11880)
  FIX: Missing Saccade information in Eyelink File (mne-tools#11877)
  Improve drawing of annotations with matplotlib (mne-tools#11855)
  MAINT: Work around NumPy deprecation (mne-tools#11878)
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for inter-figure event passing API
5 participants