ENH: Single channel annotation interaction

[nmarkowitz]

Thanks for contributing a pull request! Please make sure you have read the
contribution guidelines
before submitting.

Please be aware that we are a loose team of volunteers so patience is
necessary. Assistance handling other issues is very welcome. We value all user
contributions, no matter how minor they are. If we are slow to review, either
the pull request needs some benchmarking, tinkering, convincing, etc. or more
likely the reviewers are simply busy. In either case, we ask for your
understanding during the review process.

Again, thanks for contributing!

Reference issue

#204 , #202 , #123 , #71 , #251 , mne-python #8946

What does this implement/fix?

Adds ability to view annotations with channel specific annotations as well as click to toggle channel in annotations

Additional information

Requires a pull request be accepted into mne-python

mne-tools/mne-python#12669

Closes #123
Closes #251

[larsoner]

I took a quick look at the Notion doc. I would say don't worry about performance too much first. A wise mentor once said about code "make it work, make it pretty, make it fast". I think it makes sense to follow that pattern here. And the making it work and pretty could be a first PR. Making it fast (e.g., when a lot of annotations are present) can be another.

[nmarkowitz]

Click and toggle single channel annotations correctly working

[larsoner]

I might not get to look at the code until tomorrow, but in the meantime you can look into fixing CIs (pre-commit isn't happy at least) and then adding tests to https://github.com/mne-tools/mne-qt-browser/blob/main/mne_qt_browser/tests/test_pg_specific.py . You can see there are ways of activating fake clicks etc., hopefully you can use it to make sure that channels can be added and removed by clicking specific places

[larsoner]

... also to try this I'll need to be on the branch from mne-tools/mne-python#12669 as well, right @nmarkowitz ?

[nmarkowitz]

... also to try this I'll need to be on the branch from mne-tools/mne-python#12669 as well, right @nmarkowitz ?

That's correct

[nmarkowitz]

all tests passed locally

[larsoner]

all tests passed locally

Once you get tests working for the tests that you're modifying, make sure you run the whole test suite. Locally I get for example:

$ pytest mne_qt_browser
...
FAILED mne_qt_browser/tests/test_pg_specific.py::test_ch_specific_annot - TypeError: FillBetweenItem.__init__() missing 2 required positional arguments: 'curve1' and 'curve2'

[larsoner]

Okay existing tests pass locally, which is good. But now that you have added some new functionality, you'll need to modify test_pg_specific.py to add new tests that ensure if you do some reasonable interactions they result in reasonable changes in the annotations (or what's shown, etc.). Can you give that a shot and ping me when it's working?

[nmarkowitz]

@larsoner finished adding tests for single channel annotations. The change I made to _pg_figure.py is to allow Qt.ShiftModifier to be passed to _fake_click()

[mscheltienne]

@nmarkowitz Failure is related to the changes:

  File "/home/runner/work/mne-qt-browser/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 2320, in _toggle_single_channel_annot
    self.weakmain()._toggle_single_channel_annotation(ch_name, region_idx)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'MNEQtBrowser' object has no attribute '_toggle_single_channel_annotation'

I'll try to test both PRs tomorrow or Friday.

[larsoner]

@nmarkowitz Failure is related to the changes:

Indeed mne-tools/mne-python#12669 hasn't landed and even once it does the MNE maint/* tests will fail (which I'll update to 1.7 momentarily). The best option here I think would be to take the new tests and split them into a new test function like

def test_ch_specific_annot_interaction

(and maybe rename the existing one ..._annot_display or something). Then you can decorate the new function like:

@pytest.mark.skipif(not hasattr(mne.something.SomeClass, "_toggle_single_channel_annotation"), reason="Needs MNE 1.8+")
def test_...

or similar. Then in this test suite should pass, and once mne-tools/mne-python#12669 lands tests will run on the main version of the CIs and tests should be skipped on the maint/* version.

Make sense?

[nmarkowitz]

I think so. I'll keep trying and see how far I can get

[nmarkowitz]

@larsoner just added separate unit test with decorator you recommended

[larsoner]

Okay so first I switched to your branch and tried an example adapted from another issue somewhere and then hit a to enter annot mode:

MWE

import mne
from mne.datasets import sample
mne.viz.set_browser_backend("qt")

data_path = sample.data_path()
meg_path = data_path / "MEG" / "sample"
raw_fname = meg_path / "sample_audvis_raw.fif"
raw = mne.io.read_raw_fif(raw_fname, preload=True)
raw.pick("meg").filter(1, 30)

ch_names = ["MEG 0133", "MEG 0142", "MEG 0143", "MEG 0121"]
onsets = [1, 4.25, 7.25]
durs = [2, 1.75, 1.25]
descs = ["Specific_channels_test", 'BAD_test', 'Good_test']
chs = [ch_names, [], []]
annots = mne.Annotations(onset=onsets, duration=durs, description=descs, ch_names=chs)

raw.set_annotations(annots)
raw.plot(start=0, duration=10)

Playing around with this example I think there are two things left to fix.

1. Dealing with old MNE in a nice way for users

I forgot to update MNE-Python and did a shift-click and got an error:

/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/pyqtgraph/GraphicsScene/GraphicsScene.py:380: RuntimeWarning: Error sending click event:
Traceback (most recent call last):
  File "/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/pyqtgraph/widgets/GraphicsView.py", line 343, in mouseReleaseEvent
    super().mouseReleaseEvent(ev)
  File "/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/pyqtgraph/GraphicsScene/GraphicsScene.py", line 223, in mouseReleaseEvent
    if self.sendClickEvent(cev[0]):
  File "/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/pyqtgraph/GraphicsScene/GraphicsScene.py", line 380, in sendClickEvent
    debug.printExc("Error sending click event:")
  --- exception caught here ---
  File "/home/larsoner/python/virtualenvs/base/lib/python3.12/site-packages/pyqtgraph/GraphicsScene/GraphicsScene.py", line 378, in sendClickEvent
    item.mouseClickEvent(ev)
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 2411, in mouseClickEvent
    self._toggle_single_channel_annot(t.ch_name)
  File "/home/larsoner/python/mne-qt-browser/mne_qt_browser/_pg_figure.py", line 2320, in _toggle_single_channel_annot
    self.weakmain()._toggle_single_channel_annotation(ch_name, region_idx)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'MNEQtBrowser' object has no attribute '_toggle_single_channel_annotation'
  debug.printExc("Error sending click event:")

We should do something here instead of having this error -- maybe check for the presence of _toggle_single_channel_annotation, and if missing, emit a warning that MNE needs to be updated to support shift-clicking and short-circuit (exit the method early)?

Once you implement this, you should remove the skipif decorator you added to the test, and instead change the test to have something like:

def test_whatever(...):
    ...
    # MNE < 1.8
    if not hasattr(...):
        with pytest.warns(...):
            _fake_click(...)
            return
    ...

that way we can test on the old/stable MNE version that the warning is indeed emitted.

2. Changing behavior of shift-clicking channel name

Then I updated MNE-Python and observed:

1. Shift-clicking a channel trace did add it to the annotation (I checked it was preserved in raw when I closed the browser (:smile: ).
2. Shift-clicking a channel name did not add it to the annotation. I think it should -- you should be able to click a channel name on the left or on an actual trace and the effect should be the same here (:slightly_frowning_face:).
3. Removing all channels from a channel-specific annotation (to make it non-channel-specific) or adding a channel to a non-channel-specific annotation worked well and switch the display mode (:smile:).

So I think (2) should be fixed.

[nmarkowitz]

Shift-clicking a channel name did not add it to the annotation. I think it should -- you should be able to click a channel name on the left or on an actual trace and the effect should be the same here (:slightly_frowning_face:).

I can add this however this would have to add the channel to all annotations with the same description. So if there are two annotations with the description "event" and the channel name "EEG1" is clicked then it would be added to both annotations. How does that sound to you?

[larsoner]

Ohhh right didn't think of that. Okay never mind that part for now then!

[nmarkowitz]

I'm not sure I understand how you want the new pytest warning to function. Below is code I wrote following your template but when I run tests it says this test fails

    if not hasattr(mne.viz._figure.BrowserBase, "_toggle_single_channel_annotation"):
        with pytest.warns(UserWarning, match="MNE >= 1.8"):
            # test if shift click an existing annotation removes object
            ch_index = np.mean(annot.single_channel_annots["MEG 0133"].ypos).astype(int)
            fig._fake_click(
                ((annot_onset + annot_dur) / 2, ch_index),
                xform="data",
                button=1,
                modifier=Qt.ShiftModifier,
            )
            assert "MEG 0133" not in annot.single_channel_annots.keys()

            # test if shift click on channel adds annotation
            fig._fake_click(
                ((annot_onset + annot_dur) / 2, ch_index),
                xform="data",
                button=1,
                modifier=Qt.ShiftModifier,
            )
            assert "MEG 0133" in annot.single_channel_annots.keys()
            return

[mscheltienne]

You can use check_version from mne.utils to discriminate between code that should run for different version of a dependency, in this case different version of MNE.

What @larsoner proposed is to:

(1) In the functions you added to _pg_figure, I think on line 2320 in _toggle_single_channel_annot, check that we can actually run it else issue a warning.

if not hasattr(self.weakmain(), "_toggle_single_channel_annotation"):
    warn(...)  # use mne.utils.warn
    return  # early exit
self.weakmain()._toggle_single_channel_annotation(ch_name, region_idx)

(2) Now that you have this warning and early-exit, using a recent MNE-qt-browser with an old MNE version will not fail anymore but issue a warning. Thus, the test function that was previously decorated to be skipped can now work but with 2 different behavior depending on the MNE version: if MNE < 1.8, we look for the warning with pytest.warns and if it's above 1.8, we look for the added annotation.

def test_whatever(...):
    ...
    if not check_version("mne", "1.8"):
        with pytest.warns(RuntimeWarning, match="..."):
            ...  # action which emits the warning
            return

    # code to test for MNE 1.8 or above
    ... 

You could also separate this into 2 test function, one for before 1.8 and one for after 1.8 which would be slightly cleaner:

@pytest.mark.skipif(not check_version("mne", "1.8"), reason="Requires MNE < 1.8")
def test_single_channel_annot_old_mne():
    with pytest.warns(RuntimeWarning, match="..."):
        ...  # action which emits the warning

@pytest.mark.skipif(check_version("mne", "1.8"), reason="Requires MNE >= 1.8")
def test_single_channel_annot_new_mne():
    ...  # same action but that now works and that we test

[nmarkowitz]

ok just committed the new test

[mscheltienne]

Here you are testing that the warning you are emitting in the test function is emitted, which is not very interesting 😄 The idea is that if you run this test with MNE < 1.8 and run a _fake_click function that would add a channel specific annotation, it will issue the warning you added in _pg_figure and it won't raise an exception because it exits early (and thus skips the channel specific annotation part):

        if not hasattr(self.weakmain(), "_toggle_single_channel_annotation"):
            warn(
                "MNE must be updated to version 1.8 or above "
                "to support add/remove channels from annotation. "
            )

i.e. add the same _fake_click command which adds a channel specific annotation within the pytest.warns context manager.

[nmarkowitz]

Sorry I'm not clearly understanding. Are you thinking something like this?

    if not hasattr(mne.viz._figure.BrowserBase, "_toggle_single_channel_annotation"):
        warn(
            "MNE must be updated to version 1.8 or above "
            "to support add/remove channels from annotation. "
        )
    else:    
        # test if shift click an existing annotation removes object
        ch_index = np.mean(annot.single_channel_annots["MEG 0133"].ypos).astype(int)
        fig._fake_click(
            (4 + 2 / 2, ch_index),
            xform="data",
            button=1,
            modifier=Qt.ShiftModifier,
        )
        assert "MEG 0133" not in annot.single_channel_annots.keys()
    
        # test if shift click on channel adds annotation
        fig._fake_click(
            (4 + 2 / 2, ch_index),
            xform="data",
            button=1,
            modifier=Qt.ShiftModifier,
        )
        assert "MEG 0133" in annot.single_channel_annots.keys()

    fig.close()
    return

Should there be a context manager with pytest anywhere or does this suffice? I thought @larsoner suggested something of this form:

    if not check_version("mne", "1.8"):
        warning_message = (
            "must update MNE to >= 1.8 to test single channel annots interactions"
        )
        # emit a warning if the user tries to test single channel annots
        with pytest.warns(UserWarning, match=warning_message):
            warnings.warn(warning_message, UserWarning)
            return

[mscheltienne]

No, in _pg_figure, i.e. in the code of the browser, you should emit a warning if the version of MNE is not compatible with a feature available in the version of browser. Imagine if a users runs a browser version compatible with channel-specific annotation but with MNE 1.6. The code within _pg_figure will attempt to run the channel-specific annotation part, call self.weakmain()._toggle_single_channel_annotation and crash. This is why in the code of _pg_figure you need to check for the existence of the attribute _toggle_single_channel_annotation in self.weakmain() and issue a warning if it does not exist. This is what you did in 0bb8196 in the modification to _pg_figure and is correct.

Now, in the tests, consider the following test function which checks that a _fake_click creates a channel specific annotations:

def test_channel_specific_annotation():
    """Test that channel specific annotation works as intended."""
    raw = ...  # our raw objects that will be modified by the browser
    fig = ...  # the opened browser
    _fake_click(...)  # fake click which creates a channel specific annotation.
    # verify with an assertion that the channel specific annotations was created.
    assert "xxx" in raw.annotations.channel_specific

We have 2 scenarios: either we run this test function witth MNE 1.8, in which case it will work and pass; or we run it with an old MNE version and it will raise an error and fail. Now, since you modified _pg_figure with a warning emited for old MNE version, it will not raise an error anymore but issue the warning (and still fail as we automatically turn warnings into errors in our tests). Thus, you need to differentiate those 2 scenarios in the tests and to verify that the warning emited by _pg_figure is indeed present.

def test_channel_specific_annotation():
    """Test that channel specific annotation works as intended."""
    raw = ...  # our raw objects that will be modified by the browser
    fig = ...  # the opened browser
    if check_version("mne", "1.8"):
        _fake_click(...)  # fake click which creates a channel specific annotation.
        # verify with an assertion that the channel specific annotation was created.
        assert "xxx" in raw.annotations.channel_specific
    else:  # old MNE
        with pytest.warns(RuntimeWarning, match="MNE must be updated to version 1.8 or above"):
            _fake_click(...)  # fake click which creates a channel specific annotation, same as before.
        # verify with an assertion that the channel specific annotation was NOT created
        assert "xxx" not in raw.annotations.channel_specific  

Let us know if it's still unclear, you are getting there!
FYI, I might not be able to join today, I'm at a workshop abroad all day; and I will be on vacation for 2 weeks as of next Thursday.

[larsoner]

Rather than having to import Qt here, this would be nicer as:

Suggested change

	modifier=Qt.ShiftModifier,
	modifier="shift",

then inside _fake_click you can do something like

if modifier is not None:
    modifier = getattr(Qt, f"{modifier.capitalize()}Modifier}")

That also makes things future compatible with the matplotlib backend where eventually we might want the _fake_click there to support modifiers as well and have it work for both browsers.

[nmarkowitz]

Do you want to have modifiers as strings? I can try but it would be a bit of work. The _fake_click() function can call _mouseClick(), _mouseMove(), _mouseRelease() and _mouseDrag(). All of those, and some more functions, would have to be altered as they all accept modifier arguments. I could create a function that takes in a string or modifier and outputs the modifier. But still may be some work and for another PR.

Other than that do you think this branch is ready to be merged?

[larsoner]

Yeah we can change it as a follow-up PR I think, so I'll go ahead and merge -- thanks!