Skip to content

Conversation

@snwnde
Copy link
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

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
Copy link
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
Member

agramfort commented Dec 4, 2021

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
Contributor Author

snwnde commented Dec 4, 2021

@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
Copy link
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

Copy link
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.

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@drammock drammock merged commit d7c8fe3 into mne-tools:main Dec 6, 2021
@welcome
Copy link

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
Member

drammock commented Dec 6, 2021

thanks @snwnde!

~~~~~~~~~~~~
.. - 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
Member

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
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants