Skip to content

Fast annotation from mask#10089

Merged
drammock merged 7 commits intomne-tools:mainfrom
snwnde:main
Dec 6, 2021
Merged

Fast annotation from mask#10089
drammock merged 7 commits intomne-tools:mainfrom
snwnde:main

Conversation

@snwnde
Copy link
Copy Markdown
Contributor

@snwnde snwnde commented Dec 4, 2021

Reference issue

Enhancement #10088.

What does this implement/fix?

The function _annotations_from_mask in mne/preprocessing/artifact_detection.py
is rewritten to avoid the use of any for loop, which could be very time-consuming
when the number of components is large.

Additional information

This new implementation does not require any additional package.

@welcome
Copy link
Copy Markdown

welcome bot commented Dec 4, 2021

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

@snwnde snwnde mentioned this pull request Dec 4, 2021
Closed
@snwnde
Copy link
Copy Markdown
Contributor Author

snwnde commented Dec 4, 2021

Upon running tests, test_movement_annotation_head_correction in mne\preprocessing\tests\test_artifact_detection.py complains too big relative difference for

assert_allclose(dev_head_t_ori, dev_head_t['trans'], rtol=1e-5, atol=0)

It says:

AssertionError: 
Not equal to tolerance rtol=1e-05, atol=0

Mismatched elements: 6 / 16 (37.5%)
Max absolute difference: 5.13195626e-06
Max relative difference: 0.00025818
x: array([[ 0.995729, -0.086888,  0.031206,  0.006983],
         [ 0.090208,  0.987586, -0.128597, -0.01591 ],
         [-0.019645,  0.130863,  0.991206,  0.072583],
         [ 0.      ,  0.      ,  0.      ,  1.      ]])
y: array([[ 0.995729, -0.086889,  0.031201,  0.006983],
            [ 0.090208,  0.987586, -0.128596, -0.01591 ],
            [-0.01964 ,  0.130861,  0.991206,  0.072583],
            [ 0.      ,  0.      ,  0.      ,  1.      ]])

I fail to understand what happens despite some investigation.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Dec 4, 2021 •
edited
Loading

here are differences in annotations I get between this branch and main

# new
                       onset  duration        description
0 2015-04-20 21:38:42.981952  0.340000  BAD_mov_rotat_vel
1 2015-04-20 21:38:43.181952  2.100000       BAD_mov_dist
2 2015-04-20 21:38:45.281952  0.380000  BAD_mov_rotat_vel
3 2015-04-20 21:38:45.451952  0.120000  BAD_mov_trans_vel
4 2015-04-20 21:38:47.321952  1.000000  BAD_mov_rotat_vel
5 2015-04-20 21:38:50.311952  0.010000  BAD_mov_trans_vel
6 2015-04-20 21:38:50.311952  0.500000  BAD_mov_rotat_vel
7 2015-04-20 21:38:53.081952  0.310833  BAD_mov_rotat_vel

# old
0 2015-04-20 21:38:42.981952      0.34  BAD_mov_rotat_vel
1 2015-04-20 21:38:43.181952      2.10       BAD_mov_dist
2 2015-04-20 21:38:45.281952      0.38  BAD_mov_rotat_vel
3 2015-04-20 21:38:45.451952      0.12  BAD_mov_trans_vel
4 2015-04-20 21:38:47.321952      1.00  BAD_mov_rotat_vel
5 2015-04-20 21:38:50.311952      0.01  BAD_mov_trans_vel
6 2015-04-20 21:38:50.311952      0.50  BAD_mov_rotat_vel
7 2015-04-20 21:38:53.081952      0.31  BAD_mov_rotat_vel

I used this script

import os.path as op

from numpy.testing import assert_allclose
import numpy as np

from mne.datasets import testing
from mne.chpi import read_head_pos
from mne.io import read_raw_fif
from mne.preprocessing import (annotate_movement, compute_average_dev_head_t)


data_path = testing.data_path(download=False)
sss_path = op.join(data_path, 'SSS')
pos_fname = op.join(sss_path, 'test_move_anon_raw.pos')
raw_fname = op.join(sss_path, 'test_move_anon_raw.fif')

raw = read_raw_fif(raw_fname, allow_maxshield='yes').load_data()
pos = read_head_pos(pos_fname)

# Check 5 rotation segments are detected
annot_rot, [] = annotate_movement(raw, pos, rotation_velocity_limit=5)
assert(annot_rot.duration.size == 5)

# Check 2 translation vel. segments are detected
annot_tra, [] = annotate_movement(raw, pos, translation_velocity_limit=.05)
assert(annot_tra.duration.size == 2)

# Check 1 movement distance segment is detected
annot_dis, _ = annotate_movement(raw, pos, mean_distance_limit=.02)
assert(annot_dis.duration.size == 1)

# Check correct trans mat
raw.set_annotations(annot_rot + annot_tra + annot_dis)

print(raw.annotations.to_data_frame()

@snwnde
Copy link
Copy Markdown
Contributor Author

snwnde commented Dec 4, 2021 •
edited
Loading

@agramfort Thank you very much! By rounding the durations to decimals=2, the local test passes.

Although mne\preprocessing\tests\test_artifact_detection.py passes, there are other issues. Help is welcome!

Further investigation shows the problem occurs when the mask isn't as long as times. One can see this by adding

assert len(times) == len(mask)

in the snippet.

To maintain consistency with the old code, ends_index[ends_index >= len(times)] = len(times) - 1 is modified to ends_index[ends_index >= len(mask)] = len(mask) when len(times) > len(mask). Nevertheless, to me it makes no sense to have a mask shorter than times. Could it be a problem in the tests code?

snwnde added a commit to snwnde/mne-python that referenced this pull request Dec 4, 2021
@snwnde
round durations to 10ms mne-tools#10089
576ff93
snwnde added a commit to snwnde/mne-python that referenced this pull request Dec 4, 2021
@snwnde
maintain consistency with old code when len(times)>len(mask) mne-tool…
713d4b2 
…s#10088, mne-tools#10089
agramfort
agramfort approved these changes Dec 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

Thx @snwnde !

@larsoner i let you merge if happy.

@snwnde next time avoid opening a PR from your main branch

sappelhoff
sappelhoff approved these changes Dec 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

awesome. Thanks a lot.

@drammock @sappelhoff
more detailed changelog entry [ci skip]
67011ff 
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@drammock drammock merged commit d7c8fe3 into mne-tools:main Dec 6, 2021
@welcome
Copy link
Copy Markdown

welcome bot commented Dec 6, 2021

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@drammock
Copy link
Copy Markdown
Member

drammock commented Dec 6, 2021

thanks @snwnde!

larsoner
larsoner reviewed Dec 6, 2021
View reviewed changes
doc/changes/latest.inc
~~~~~~~~~~~~
.. - Add something cool (:gh:`9192` **by new contributor** |New Contributor|_)

- Speed up :func:`mne.preprocessing.annotate_muscle_zscore`, :func:`mne.preprocessing.annotate_movement`, and :func:`mne.preprocessing.annotate_nan` through better annotation creation (:gh:`10089` **by new contributor** `Senwen Deng`_)
Copy link
Copy Markdown
Member

@larsoner larsoner Dec 6, 2021

Choose a reason for hiding this comment

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

Should be:

.. |Senwen Deng| replace:: **Senwen Deng**


- ... |Senwen Deng|_)

@drammock can you open a quick PR or push to main to fix it?

Copy link
Copy Markdown
Member

@drammock drammock Dec 6, 2021

Choose a reason for hiding this comment

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

oops. yes, will fix.

larsoner added a commit to GuillaumeFavelier/mne-python that referenced this pull request Dec 6, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into enh/coreg_feats
28773f4 
* upstream/main:
  Use fixes._compare_version for version checks everywhere (mne-tools#10091)
  Fast annotation from mask (mne-tools#10089)
  fix trace offsets in butterfly mode (mne-tools#10087)
  fix plot_compare_evokeds topo legend axes placement (mne-tools#9927)
  doc: clarify ica.apply include and exclude params (mne-tools#10086)
  MRG: Make y a required parameter in CSP.fit_transform() (mne-tools#10084)
  Add scrollbar to report tag dropdown menu (mne-tools#10082)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drammock drammock drammock left review comments

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

@sappelhoff sappelhoff sappelhoff approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@snwnde @agramfort @drammock @larsoner @sappelhoff