Improve drawing of annotations with matplotlib

[mscheltienne]

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

[mscheltienne]

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.

[larsoner]

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]

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]

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]

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]

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]

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]

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]

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]

@drammock should come back green now 🤞

[mscheltienne]

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

[larsoner]

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 !