Proposal for inter-figure event passing API #11678

Activity

1. wmvanvliet

   added

   ENH

   on May 5, 2023
2. wmvanvliet

   assigned

   agramfort

   ,

   larsoner

   and

   wmvanvliet

   on May 5, 2023
3. larsoner commented on May 5, 2023

   

   larsoner

   on May 5, 2023

   Member

   More actions

   The proposal looks reasonable. A few minor comments:

   1. Looks pretty simple already!

   2. When subscribing, the API allows to pass
      on some additional parameters that to be passed along to the callback function.

      I don't think we need this, people should just use functools.partial.

   3. All we need to decide now is the public API and everything else should be private (like the global event dict or whatever).

   4. Implementation wise, we could have a single event channel object that is shared between figures. However, that makes it difficult to ever "unlink" channels. Unlinking is something that needs to happen for example when the user closes one of the figures.

      I'm not sure that's explicitly needed. The handler for the figure can use if self.closed: return style logic for the figure-closed case. And as long as we weakref the callback then it shouldn't prevent GC. But maybe I'm missing something.

   5. You use global event_channels in several places but I don't think you need it. If event_channels is mutable (like dict) then you can just assign vars etc. (it'll be in the __closure__ of the functions). The only time you really need global is for assignment like event_channels = ..., but as long as you do that at the module level rather than within a function you're good.
4. larsoner

   added this to the 1.5 milestone on May 5, 2023
5. wmvanvliet commented on May 6, 2023

   

   wmvanvliet

   on May 6, 2023

   ContributorAuthor

   More actions

   1. Thanks!
   2. Fair enough. That makes the API even simpler (edit done in the gist)
   3. Yes, but its good to have an actual implementation as well so we can make sure what we propose as API actually works and covers all use cases.
   4. You mean every event handler would need if self.closed: return boilerplate added to it? That doesn't sound like a good tradeoff.
   5. Correct. Changed in Gist.
6. larsoner commented on May 8, 2023

   

   larsoner

   on May 8, 2023

   Member

   More actions

   I think my last comment is that we have to avoid the use of bare "event" here because it already means something in MNE. Maybe FigEvent or UIEvent or something similar.

   @drammock can you look at the gist linked at the top and see if you like the structure? I think I'm happy 👍
7. drammock commented on May 8, 2023

   

   drammock

   on May 8, 2023

   Member

   More actions

   I didn't have a chance to try it out yet, will do tomorrow. The plan generally looks good to me though.

   questions

   1. Should sources for Event include our pyqtgraph-based raw browser?
   2. Nitpick: I think best practice is fig.canvas.draw_idle() (not .draw()) right?
   3. Can we do all this as methods on the figure objects rather than functions in the (already super crowded) MNE namespace?

   comment (after hardly any thought about it): I'd probably start off doing it as a mixin; there are some existing figure wrappers in mne/viz/_mpl_figure.py that could be leveraged too.
8. drammock commented on May 8, 2023

   

   drammock

   on May 8, 2023

   Last edited by drammock

   Member

   More actions

   actually just tried it out. Super slick! Had to add a plt.ion() to get both figs to show. Got this traceback when closing the brain figure:

   Traceback (most recent call last):
     File "<ipython-input-1-125383a73ddd>", line 123, in delete_event_channel
       publish(fig, event='close')  # Notify subscribers of imminent close
     File "<ipython-input-1-125383a73ddd>", line 165, in publish
       if event.name not in channel:
   AttributeError: 'str' object has no attribute 'name'
   
9. larsoner commented on May 9, 2023

   

   larsoner

   on May 9, 2023

   Member

   More actions

   Should sources for Event include our pyqtgraph-based raw browser?

   I think we should make as much stuff as helpful publish or subscribe, including matplotlib figures, the mne-qt-browser (possibly at a level of abstraction that it works for both MPL and PyQtGraph-based backends), Brain, etc. You could imagine it could be useful to have two mne-qt-browser instances browse two different Raw instances simultaneously, and we get it easily with this framework I think

   Can we do all this as methods on the figure objects rather than functions in the (already super crowded) MNE namespace?

   I'm guessing no because it would require subclassing everything, including stuff like the figure returned by plot_evoked. So it makes it less general/easy I think. But I do think the functions should live in mne.viz, that way mne. can be less crowded.

   👍React with 👍1Reacted by Marijn van Vliet
10. larsoner

    modified the milestones: 1.5, 1.6 on Jul 31, 2023
11. larsoner

    closed this as completedin #11685on Aug 16, 2023