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.

Sign up for GitHub

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

Jump to bottom

VIZ, ENH: allow show/hide annotations by label #8624

Merged
merged 9 commits into from
Dec 12, 2020
Merged

VIZ, ENH: allow show/hide annotations by label #8624

merged 9 commits into from
Dec 12, 2020

Conversation

drammock
Copy link
Member

@drammock drammock commented Dec 7, 2020

closes #8578

@drammock
Copy link
Member Author

drammock commented Dec 7, 2020

demo

@drammock
Copy link
Member Author

drammock commented Dec 7, 2020

@larsoner @hoechenberger @cbrnr One question I had when implementing this is: what should happen if someone toggles an annotation label to be invisible, but that label is still "selected", and then they click-drag on the plot to create a new annotation? Currently what happens is the span selector shows during dragging, but the new annotation disappears as soon as the mouse button is released. I don't think that's ideal, but the only alternative I could think of was for all annotations with that label to become visible when the mouse button is either pressed or released, either of which seem kinda weird from a UX point of view. Thoughts?

@cbrnr
Copy link
Contributor

cbrnr commented Dec 8, 2020

I'd deselect it before hiding. It doesn't even have to be reselected when showing the type again.

@agramfort
Copy link
Member

agramfort commented Dec 8, 2020 via email

@hoechenberger
Copy link
Member

hoechenberger commented Dec 8, 2020 •
edited
Loading

Great!!!

Using the MacOSX backend here. Running into two issues:

  • in fullscreen mode (tiling: Raw plot left, Annotations options right), something goes wrong when adding a new Annotation label. This problem already existed in master
  • something is off with the checkboxes. This also happens outside of fullscreen view.

ezgif com-video-to-gif

@drammock
Copy link
Member Author

drammock commented Dec 8, 2020

@hoechenberger I don't know the details of how macOS's tiling works, but I wouldn't necessarily expect it to work smoothly, given that the figure is re-created each time a label is added. Anything you can tell me about it that might help figure out what is going wrong? The checkbox duplication and sizing is also weird; any ideas there?

Regarding how to handle adding an annotation when its label's visibility is turned off: after some reflection I think I prefer @agramfort's suggested fix (automatically switch visibility to true for that label) rather than @cbrnr's suggestion (automatically de-activate a label when it's made invisible). The latter proposal forces us to handle a new situation: no annotations visible means no labels are selected (currently there is always one label selected), so we'd need to special-case what the SpanSelector should (not) do when no label is selected. Certainly possible, but I think "auto-make-visible" a label is easier to implement, and about equal in terms of user expectations of what should happen in that corner case. Ayone else want to weigh in?

@cbrnr
Copy link
Contributor

cbrnr commented Dec 9, 2020 •
edited
Loading

I can see the problem and I'm fine with @agramfort's suggestion. However, this means that at least one annotation type is always visible. I think it would be useful to hide all annotations, because sometimes annotations get in the way of the actual time series (especially in a data set with many different annotations). I don't know the best solution, but having a separate show/hide annotations would be one option (which is relatively independent from this PR).

@agramfort
Copy link
Member

agramfort commented Dec 9, 2020 via email

@drammock
Copy link
Member Author

drammock commented Dec 9, 2020

It's only when you add one that all annot of this type become visible

Yes, this is what I had in mind. I'll move forward with that approach.

@larsoner
Copy link
Member

@hoechenberger do you want to test it?

larsoner
larsoner approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Code changes LGTM but it would be good if a heavy annotation user could test it out

@larsoner larsoner added this to the 0.22 milestone Dec 11, 2020
@hoechenberger
Copy link
Member

@larsoner Yes I will have a look shortly.

hoechenberger
hoechenberger approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Works great for me!

@hoechenberger
Copy link
Member

It's only when you add one that all annot of this type become visible

Yes, this is what I had in mind. I'll move forward with that approach.

This isn't working for me yet, not sure if it was supposed to :)

@drammock
Copy link
Member Author

It's only when you add one that all annot of this type become visible

Yes, this is what I had in mind. I'll move forward with that approach.

This isn't working for me yet, not sure if it was supposed to :)

working locally but I haven't pushed yet. Decided to see if I could also get useblit=True to work on the spanselector.

@drammock
Copy link
Member Author

ok, I didn't manage to get useblit=True working for the span selector, but can do that in a separate PR. But I did get the checkboxes to stay vertically aligned with their corresponding buttons, and I got them to stop migrating rightward with each new label added :) They still change size a little bit, but I minimized that as much as I could. Ready for one final look @hoechenberger

hoechenberger
hoechenberger approved these changes Dec 12, 2020
View reviewed changes
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Perfect! I love it 😍

@hoechenberger
Copy link
Member

@drammock Can you add a changelog entry please? Otherwise, LGTM!!

@hoechenberger hoechenberger merged commit cfd5659 into mne-tools:master Dec 12, 2020
@hoechenberger
Copy link
Member

Thanks a bunch, @drammock!

larsoner added a commit to agramfort/mne-python that referenced this pull request Dec 16, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into fix_rstrip_edf
84c6e81 
* upstream/master: (42 commits)
  MRG, ENH: Add DICS bias tests (mne-tools#8610)
  MRG, BUG, ENH: Add window option (mne-tools#8662)
  BUG: Fix alpha for volumes (mne-tools#8663)
  MRG, BUG: Fix bugs with envcorr (mne-tools#8658)
  MRG, ENH: Progressbar for csd_morlet (mne-tools#8608)
  Render is necessary now (mne-tools#8657)
  VIZ: Fix head size (mne-tools#8651)
  MRG, MAINT: bump sphinxcontrib-bitex version (mne-tools#8653)
  MRG, MAINT: Improve server env (mne-tools#8656)
  BUG: Mayavi center (mne-tools#8644)
  VIZ, ENH: allow show/hide annotations by label (mne-tools#8624)
  Add regression test for EEGLAB data with a chanlocs struct (mne-tools#8647)
  FIX: scalar_bar (mne-tools#8643)
  MRG: Small fix to tutorial; rename plot_events ordinate label to "Event id"; improve some SSP docstrings (mne-tools#8612)
  MRG, ENH: make plot alignment use defaults for colors (mne-tools#8553)
  BUG: Fix passing of channel type (mne-tools#8638)
  FIX: fixed loop over norm PSF/CTF options (mne-tools#8636)
  MRG, BUG: Pass kwargs (mne-tools#8630)
  DOC: Clearer error message (mne-tools#8631)
  BUG: Fix number of labels (mne-tools#8629)
  ...
larsoner added a commit to wmvanvliet/mne-python that referenced this pull request Dec 16, 2020
@larsoner
Merge remote-tracking branch 'upstream/master' into dics_rank
fc59492 
* upstream/master: (38 commits)
  MRG, ENH: Add DICS bias tests (mne-tools#8610)
  MRG, BUG, ENH: Add window option (mne-tools#8662)
  BUG: Fix alpha for volumes (mne-tools#8663)
  MRG, BUG: Fix bugs with envcorr (mne-tools#8658)
  MRG, ENH: Progressbar for csd_morlet (mne-tools#8608)
  Render is necessary now (mne-tools#8657)
  VIZ: Fix head size (mne-tools#8651)
  MRG, MAINT: bump sphinxcontrib-bitex version (mne-tools#8653)
  MRG, MAINT: Improve server env (mne-tools#8656)
  BUG: Mayavi center (mne-tools#8644)
  VIZ, ENH: allow show/hide annotations by label (mne-tools#8624)
  Add regression test for EEGLAB data with a chanlocs struct (mne-tools#8647)
  FIX: scalar_bar (mne-tools#8643)
  MRG: Small fix to tutorial; rename plot_events ordinate label to "Event id"; improve some SSP docstrings (mne-tools#8612)
  MRG, ENH: make plot alignment use defaults for colors (mne-tools#8553)
  BUG: Fix passing of channel type (mne-tools#8638)
  FIX: fixed loop over norm PSF/CTF options (mne-tools#8636)
  MRG, BUG: Pass kwargs (mne-tools#8630)
  DOC: Clearer error message (mne-tools#8631)
  BUG: Fix number of labels (mne-tools#8629)
  ...
@drammock drammock deleted the raw-annot-toggle branch December 29, 2020 16:53
@drammock drammock mentioned this pull request May 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoechenberger hoechenberger hoechenberger approved these changes

@larsoner larsoner larsoner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.22
Development

Successfully merging this pull request may close these issues.

Toggle Annotations visibility in plot_raw()
5 participants
@drammock @cbrnr @agramfort @hoechenberger @larsoner