Improve drawing of annotations with matplotlib by mscheltienne · Pull Request #11855 · mne-tools/mne-python

mscheltienne · 2023-08-02T09:30:18Z

Looks like I'm going again for an intertwined PR with mne-tools/mne-qt-browser#178 😟
Changes:

Use blitting for faster render in matplotlib. This TODO item seems outdated. It's been in since 0.22 (I think?) and I don't see any additional bug compared to useblit=False anymore. Maybe fixed upstream? The improvement is drastic.
Fix wrapping of annotations around when you exit the view/plot area. The issue is present both with or without blitting, but it's emphasized with blitting.

import numpy as np
from mne import create_info
from mne.io import RawArray
from mne.viz import set_browser_backend


set_browser_backend("matplotlib")
data = np.random.randn(3, 100000)
raw = RawArray(data, create_info(3, 1000, "eeg"))
raw.plot(theme="light", scalings="auto")

On the screencast, I move my mouse on the channel scrollbar on the right, which yields this bug.

Screencast.from.08-02-2023.11.20.31.AM.webm

Fix merge of dragged annotations. Annotations can not be dragged in the matplotlib backend while it is possible in the Qt backend. The function used to merge annotations does not support merge of dragged annotations, only of newly created annotations which overlap with another or of resized annotations which yields an overlap. It lacks knowledge of which annotation was modified within mne.inst.annotations and doesn't support changing both boundaries at once.
(optional) Enable dragging of annotations in the maptlotlib backend
Fix warning emitted when no annotation-label exists. Before, a traceback would be yielded because buttons was set to None, and thus it doesn't have the attribute labels.
(optional) Disable the span selector when no annotation-label exists.

EDIT:

Fix addition of more than one label. For now, it raises:


Traceback (most recent call last):
  File "/home/scheltie/pyvenv/mscheltienne/mne-python/lib/python3.10/site-packages/matplotlib/cbook/__init__.py", line 309, in process
    func(*args, **kwargs)
  File "/home/scheltie/pyvenv/mscheltienne/mne-python/lib/python3.10/site-packages/matplotlib/widgets.py", line 1114, in _clear
    self.ax.draw_artist(self._checks)
  File "/home/scheltie/pyvenv/mscheltienne/mne-python/lib/python3.10/site-packages/matplotlib/axes/_base.py", line 3074, in draw_artist
    a.draw(self.figure.canvas.get_renderer())
  File "/home/scheltie/pyvenv/mscheltienne/mne-python/lib/python3.10/site-packages/matplotlib/artist.py", line 72, in draw_wrapper
    return draw(artist, renderer)
  File "/home/scheltie/pyvenv/mscheltienne/mne-python/lib/python3.10/site-packages/matplotlib/collections.py", line 971, in draw
    self.set_sizes(self._sizes, self.figure.dpi)
AttributeError: 'NoneType' object has no attribute 'dpi'

Might be related to matplotlib/matplotlib#21569

Screencast.from.08-02-2023.01.13.29.PM.webm

…or dragging the annotation directly

mscheltienne · 2023-08-03T14:11:16Z

mne/viz/tests/test_raw.py

+    Let's not bother in figuring out on which sample the _fake_click actually
+    dropped the annotation, especially with the 600.614 Hz weird sampling rate.
+    -> atol = 10 / raw.info["sfreq"]


If you have a better idea to get that test working, I'm listening. In mne-tools/mne-qt-browser#178, it failed the Ubuntu and Windows CIs (link) with atol = 2 / raw.info["sfreq"]. Locally, on an ubuntu-based distro it was passing..

It looks to me like the _fake_click location is not very precise. I tried changing the onset/duration/click locations with values corresponding to an exact sample, without luck. For instance, I tried to click on the right edge of an annotation and it was working.. within +/- 15 samples of the actual edge location.

It looks to me like the _fake_click location is not very precise

Yes it's possible and I think it's okay to have a loose tolerance. As long as it's strict enough to catch if we broke things it should be fine. We can always improve the tolerance later

mscheltienne · 2023-08-03T14:19:46Z

In this PR:

Fixes warning emitted when no annotation-label exist
Refactor _update_annotation_segments to avoid the for loop
Apply pending TODO item (req. matplotlib > 3.0) for inline bold formatting of the instructions
Enable blitting for faster render
Add a test for Merge annotations which are subject to a move/shrink/extend yielding an overlap with other annotations of the same type mne-qt-browser#178

I did not add a "Draggable" behavior for annotations to the MPL backend. It looks to me like a lot of work and would require something similar to the DraggableLine defined here but for the fill area defined here.

I will re-raise the 2 other bugs (traceback yielded when adding labels + wrapping around when exiting the main ax area) in other issues. I don't know how to fix them at this time, and neither is blocking for this PR.

mscheltienne · 2023-08-03T14:24:57Z

And CIs should be failing here due to the added test which can not pass without mne-tools/mne-qt-browser#178

larsoner

@drammock I'll let you merge if happy

@mscheltienne just double-checking -- the top comment has this un-checked box:

Fix wrapping of annotations around when you exit the view/plot area. The issue is present both with or without blitting, but it's emphasized with blitting.

Should we wait to merge or is it good to go from your end?

mscheltienne · 2023-08-03T15:47:51Z

I have no clue how to fix it for now, so I will re-raise it in a separate issue to keep track. But it's not blocking at the moment. Good to merge on my end.

drammock · 2023-08-03T19:48:44Z

the macOS ARM failure looks vaguely related (to other recent PRs at least):

mne/viz/tests/test_raw.py:857: in test_merge_annotations
    assert len(raw.annotations) == 5
E   assert 6 == 5
E    +  where 6 = len(<Annotations | 6 segments: bad_test (3), test (3)>)
E    +    where <Annotations | 6 segments: bad_test (3), test (3)> = <Raw | test_raw.fif, 9 x 14400 (24.0 s), ~3.9 MB, data loaded>.annotations

is it an issue of the CI config not using the right version of mne-qt-browser?

mscheltienne · 2023-08-03T20:00:35Z

Yes, it's not running mne-qt-browser main:

└☑ mne-qt-browser       0.5.2
        /opt/homebrew/lib/python3.10/site-packages/mne_qt_browser

We could mark the test as xfail for qt-browser version below 0.5.3.

drammock · 2023-08-03T20:28:04Z

We could mark the test as xfail for qt-browser version below 0.5.3.

let's do that... I don't know how long it will take for #11859 to resolve and I don't want Cirrus failure on every PR between now and then.

larsoner · 2023-08-03T22:07:28Z

let's do that... I don't know how long it will take for #11859 to resolve and I don't want Cirrus failure on every PR between now and then.

That PR is very close to done... but even then it would be good to have that xfail so that other devs running tests who haven't updated to main of mne-qt-browser can still have things pass.

larsoner · 2023-08-03T23:03:42Z

@drammock should come back green now 🤞

mscheltienne · 2023-08-04T08:10:09Z

Thank you @larsoner for the CIs fixes!

drammock

the test is not very DRY but I'm not sure it would actually be easier to follow if it were DRY. +1 for merge after 1 tiny wording change to the xfail message

mne/viz/tests/test_raw.py

larsoner · 2023-08-14T16:11:41Z

Will commit suggestion and merge main into the branch once #11878 lands, marking for merge when green. Thanks in advance and thanks for your patience @mscheltienne !

Co-authored-by: Daniel McCloy <dan@mccloy.info>

* upstream/main: 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)

* 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)

improve drawing of annotations with mpl

6e1eacd

mscheltienne marked this pull request as draft August 2, 2023 09:30

improve instructions [ci skip]

96fdad2

mscheltienne mentioned this pull request Aug 2, 2023

Merge annotations which are subject to a move/shrink/extend yielding an overlap with other annotations of the same type mne-tools/mne-qt-browser#178

Merged

mscheltienne added 4 commits August 3, 2023 12:47

Merge remote-tracking branch 'upstream/main' into annotations [ci skip]

60773b6

avoid for loop [ci skip]

453f108

add test for merging annotations in the Qt backend by dragging edges …

d4cdb2d

…or dragging the annotation directly

fix test by using different values

60a9f86

mscheltienne mentioned this pull request Aug 3, 2023

Run CIs on main and on mscheltienne/mne-python:annotations mne-tools/mne-qt-browser#185

Closed

mscheltienne added 2 commits August 3, 2023 15:32

give 2 sample of freedom to handle the click between samples

903a4f0

give 10 samples of freedom to balance the imprecise _fake_click

f2aa3b2

mscheltienne commented Aug 3, 2023

View reviewed changes

mscheltienne marked this pull request as ready for review August 3, 2023 14:12

Merge branch 'main' into annotations

6aa9fdd

larsoner approved these changes Aug 3, 2023

View reviewed changes

mscheltienne and others added 3 commits August 3, 2023 17:50

trigger cis

3f6b63c

FIX: Use main mne-qt-browser

c2e0a7b

FIX: pip

688bb79

larsoner added 2 commits August 3, 2023 18:59

Merge branch 'main' into annotations

6d07ba3

FIX: xfail

a518dbd

FIX: More

93f8601

mscheltienne added this to the 1.5 milestone Aug 4, 2023

larsoner added 2 commits August 7, 2023 12:31

Merge branch 'main' into annotations

bd7da64

Merge branch 'main' into annotations

2319763

drammock approved these changes Aug 14, 2023

View reviewed changes

mne/viz/tests/test_raw.py Outdated Show resolved Hide resolved

larsoner enabled auto-merge (squash) August 14, 2023 16:11

larsoner and others added 2 commits August 14, 2023 12:11

Update mne/viz/tests/test_raw.py [ci skip]

491fdac

Co-authored-by: Daniel McCloy <dan@mccloy.info>

Merge branch 'main' into annotations

2bf5775

larsoner merged commit 6f9b03c into mne-tools:main Aug 14, 2023

mscheltienne deleted the annotations branch August 15, 2023 12:02

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024

Improve drawing of annotations with matplotlib (mne-tools#11855)

b4e75c2

Uh oh!

Conversation

mscheltienne commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscheltienne Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

larsoner Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

mscheltienne commented Aug 3, 2023

Uh oh!

mscheltienne commented Aug 3, 2023

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

mscheltienne commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drammock commented Aug 3, 2023

Uh oh!

mscheltienne commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drammock commented Aug 3, 2023

Uh oh!

larsoner commented Aug 3, 2023

Uh oh!

larsoner commented Aug 3, 2023

Uh oh!

mscheltienne commented Aug 4, 2023

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

larsoner commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mscheltienne commented Aug 2, 2023 •

edited

Loading

mscheltienne commented Aug 3, 2023 •

edited

Loading

mscheltienne commented Aug 3, 2023 •

edited

Loading