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

ENH: Single channel annotation interaction #255

Merged
merged 29 commits into from
Jun 28, 2024
Merged

ENH: Single channel annotation interaction #255

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6fa53e5
fix bugs
nmarkowitz May 31, 2024
d1b556f
visibility selection toggle update
nmarkowitz Jun 3, 2024
4c97dcc
Merge branch 'main' into ENH-single-channel-annotation
nmarkowitz Jun 5, 2024
52b5a9d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 6, 2024
c425376
create new class for single channel annot
nmarkowitz Jun 6, 2024
f2ba689
Merge branch 'ENH-single-channel-annotation' of https://github.com/nm…
nmarkowitz Jun 6, 2024
30567a4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 6, 2024
64ec5b1
toggle SingleChannelAnnot in viewbox
nmarkowitz Jun 10, 2024
febfc98
add toggle method for single chan annot
nmarkowitz Jun 12, 2024
fb1470b
update annotregion toggle single chan annot
nmarkowitz Jun 13, 2024
94e9663
click toggling correctly
nmarkowitz Jun 17, 2024
b13a246
Merge pull request #1 from nmarkowitz/ENH-single-channel-annotation-c…
nmarkowitz Jun 17, 2024
4473d73
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 17, 2024
4531cb5
Merge branch 'mne-tools:main' into ENH-single-channel-annotation
nmarkowitz Jun 17, 2024
ce625dc
Merge branch 'main' into ENH-single-channel-annotation
nmarkowitz Jun 20, 2024
b03eb30
TST: Ping
larsoner Jun 20, 2024
5713964
TST: Ping
larsoner Jun 20, 2024
7688ca4
update SingleChannelAnnot slide
nmarkowitz Jun 21, 2024
0c72430
Update mne_qt_browser/_pg_figure.py
larsoner Jun 21, 2024
fc1ab80
basic single chan annot tests added
nmarkowitz Jun 24, 2024
735b56b
add interactive single channel annot tests
nmarkowitz Jun 24, 2024
ed93017
Merge branch 'ENH-single-channel-annotation' of https://github.com/nm…
nmarkowitz Jun 24, 2024
2ed88ad
Update test_pg_specific.py
nmarkowitz Jun 24, 2024
1763088
Merge branch 'main' into ENH-single-channel-annotation
larsoner Jun 25, 2024
cb89927
add test for ch specific interaction
nmarkowitz Jun 25, 2024
ebc495d
Merge branch 'mne-tools:main' into ENH-single-channel-annotation
nmarkowitz Jun 25, 2024
0bb8196
update test for ch_spec_annot
nmarkowitz Jun 26, 2024
76921e1
change spec chan annot test
nmarkowitz Jun 27, 2024
c32fd84
hack at unit test
nmarkowitz Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions mne_qt_browser/_pg_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2316,6 +2316,14 @@ def _remove_single_channel_annot(self, ch_name):

def _toggle_single_channel_annot(self, ch_name):
"""Add or remove single channel annotations."""
# Exit if mne-python not updated to support shift-click
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. "
)
return

region_idx = self.weakmain()._get_onset_idx(self.getRegion()[0])
self.weakmain()._toggle_single_channel_annotation(ch_name, region_idx)
if ch_name not in self.single_channel_annots.keys():
Expand Down
38 changes: 15 additions & 23 deletions mne_qt_browser/tests/test_pg_specific.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
#
# License: BSD-3-Clause

import mne.viz._figure
import warnings

import numpy as np
import pytest
from mne import Annotations
from mne.utils import check_version
from numpy.testing import assert_allclose
from qtpy.QtCore import Qt
from qtpy.QtTest import QTest
Expand Down Expand Up @@ -101,7 +103,7 @@ def test_annotations_interactions(raw_orig, pg_backend):
fig.msg_box.close()


def test_ch_specific_annot_display(raw_orig, pg_backend):
def test_ch_specific_annot(raw_orig, pg_backend):
"""Test plotting channel specific annotations."""
ch_names = ["MEG 0133", "MEG 0142", "MEG 0143", "MEG 0423"]
annot_onset, annot_dur = 1, 2
Expand Down Expand Up @@ -145,30 +147,20 @@ def test_ch_specific_annot_display(raw_orig, pg_backend):
assert annot_dock.start_bx.value() == 4
assert single_channel_annot.lower.xData[0] == 4

fig.close()


@pytest.mark.skipif(
not hasattr(mne.viz._figure.BrowserBase, "_toggle_single_channel_annotation"),
reason="needs MNE 1.8",
)
def test_ch_specific_annot_interactions(raw_orig, pg_backend):
"""Test interactiveness and responsiveness of channel specific annotations."""
ch_names = ["MEG 0133", "MEG 0142", "MEG 0143", "MEG 0423"]
annot_onset, annot_dur = 1, 2
annots = Annotations([annot_onset], [annot_dur], "some_chs", ch_names=[ch_names])
raw_orig.set_annotations(annots)

fig = raw_orig.plot()
fig.test_mode = True
annot = fig.mne.regions[0]

fig._fake_keypress("a") # activate annotation mode
# MNE >= 1.8
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
Copy link
Member

@mscheltienne mscheltienne Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mscheltienne mscheltienne Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


# 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),
(4 + 2 / 2, ch_index),
xform="data",
button=1,
modifier=Qt.ShiftModifier,
Expand All @@ -177,7 +169,7 @@ def test_ch_specific_annot_interactions(raw_orig, pg_backend):

# test if shift click on channel adds annotation
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

fig._fake_click(
((annot_onset + annot_dur) / 2, ch_index),
(4 + 2 / 2, ch_index),
xform="data",
button=1,
modifier=Qt.ShiftModifier,
Expand Down