[MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg by sappelhoff · Pull Request #9556 · mne-tools/mne-python

sappelhoff · 2021-07-12T16:06:09Z

closes #9548

~~Work in progress~~

To do

sappelhoff

CircleCI is failing, see screenshot below - if somebody immediately knows why, a hint would be appreciated :-)

mne/io/base.py

sappelhoff · 2021-07-13T13:50:39Z

I think I found a bug along the way, see this docstr of find_bads_ecg:

mne-python/mne/preprocessing/ica.py

Lines 1251 to 1256 in 308a0ab

    
                   start : int | float | None 
        
                       First sample to include. If float, data will be interpreted as 
        
                       time in seconds. If None, data will be used from the first sample. 
        
                   stop : int | float | None 
        
                       Last sample to not include. If float, data will be interpreted as 
        
                       time in seconds. If None, data will be used to the last sample.

start and stop are then passed to _make_ecg just here:

mne-python/mne/preprocessing/ica.py

Lines 1305 to 1308 in 308a0ab

    
           if idx_ecg is None: 
        
               ecg, times = _make_ecg(inst, start, stop, 
        
                                      reject_by_annotation=reject_by_annotation) 
        
           else:

Then in _make_ecg, weird things happen: If the instance is Epochs, .crop is used, interpreting start and stop as seconds (even if they might have been int, so sample indices, not seconds) ... if it's Raw, these are passed to start and stop of raw.get_data, regardless of whether they are int (sample indices, that would be fine) or float (which would raise an error, because raw.get_data start and stop cannot deal with floats)

mne-python/mne/preprocessing/ecg.py

Lines 379 to 385 in 308a0ab

    
           if isinstance(inst, BaseRaw): 
        
               reject_by_annotation = 'omit' if reject_by_annotation else None 
        
               ecg, times = inst.get_data(picks, start, stop, reject_by_annotation, 
        
                                          True) 
        
           elif isinstance(inst, BaseEpochs): 
        
               ecg = np.hstack(inst.copy().crop(start, stop).get_data()) 
        
               times = inst.times

In particular, the error raised would be:

TypeError: slice indices must be integers or None or have an index method

Just try it yourself by modifying our ICA tutorial, add a tstart=2 to these lines:

mne-python/tutorials/preprocessing/40_artifact_correction_ica.py

Lines 402 to 404 in 308a0ab

    
           # find which ICs match the ECG pattern 
        
           ecg_indices, ecg_scores = ica.find_bads_ecg(raw, method='correlation', 
        
                                                       threshold='auto')

That should work. But now modify it to tstart=2.3 (or any other float), which raises an error although according to the docstr it shouldn't.

mne/io/base.py

mne/preprocessing/ecg.py

mne/io/tests/test_raw.py

mne/io/base.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

…thon into enh/get_data/tmin_tmax

mne/preprocessing/ecg.py

sappelhoff

Okay, I think I am done here (ready for review/merge). I have one failure that I can't get to the bottom to: mne/tests/test_epochs.py::test_own_data FAILED [ 50%]

mne/tests/test_epochs.py:517: in test_own_data
    assert epochs._data.flags['OWNDATA']
E   assert False

One more point: I am using the following short snippet two-and-a-half times:

        _validate_type(tmin, types=('numeric', None), item_name='tmin',
                       type_name="float, None")
        _validate_type(tmax, types=('numeric', None), item_name='tmax',
                       type_name='float, None')

        # handle tmin/tmax as start and stop indices into data array
        n_times = self.times.size
        start = 0 if tmin is None else self.time_as_index(tmin)[0]
        stop = n_times if tmax is None else self.time_as_index(tmax)[0]

        # truncate start/stop to the open interval [0, n_times]
        start = min(max(0, start), n_times)
        stop = min(max(0, stop), n_times)

Should I turn that into a private function in mne.utils and import + use it? Or is it reasonably short / repeated few enough times to just ignore "DRY"? :-)

larsoner · 2021-07-15T19:38:09Z

I would make it a private method in the TimeMixin in base.py, DRY and scoped to the same level as time_as_index (which it uses) which is nice

doc/changes/latest.inc

mne/epochs.py

…thon into enh/get_data/tmin_tmax

sappelhoff · 2021-07-16T11:47:12Z

I would make it a private method in the TimeMixin in base.py, DRY and scoped to the same level as time_as_index (which it uses) which is nice

agreed, thanks for the suggestion, this is much better!

larsoner · 2021-07-16T18:33:58Z

Thanks @sappelhoff !

hoechenberger · 2021-07-16T18:41:35Z

Amazing, thanks, @sappelhoff!

* upstream/main: [MRG, ENH] Find aseg labels near montage (mne-tools#9545) Add label to colorbar in GAT plot [skip actions] (mne-tools#9582) [ENH, MRG] Encapsulate warp elec image in function (mne-tools#9544) [DOC, MRG] Add "info" to `docdict` (mne-tools#9574) [MRG] Add `units` parameter to get_data for Evoked (mne-tools#9578) [MRG, ENH] Get annotation description from snirf stim name (mne-tools#9575) [MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg (mne-tools#9556)

enh: add tmin/tmax to raw.get_data

2c50524

sappelhoff added the ENH label Jul 12, 2021

sappelhoff mentioned this pull request Jul 12, 2021

Add get_data method to Evoked #9555

Merged

2 tasks

sappelhoff commented Jul 12, 2021

View reviewed changes

drammock reviewed Jul 12, 2021

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

sappelhoff added 4 commits July 13, 2021 12:31

Merge branch 'main' into enh/get_data/tmin_tmax

598fd25

use double ticks in docstr

cc08e4a

fix: start default is 0, not None

74b40ca

add tests for raw

130a372

sappelhoff added the FIX label Jul 13, 2021

validate types, reveal bug in _make_ecg

684f51f

larsoner reviewed Jul 13, 2021

View reviewed changes

mne/io/base.py Outdated Show resolved Hide resolved

mne/preprocessing/ecg.py Outdated Show resolved Hide resolved

mne/io/tests/test_raw.py Outdated Show resolved Hide resolved

mne/io/base.py Show resolved Hide resolved

mne/io/base.py Outdated Show resolved Hide resolved

sappelhoff and others added 6 commits July 14, 2021 10:04

Merge branch 'main' into enh/get_data/tmin_tmax

e4745bd

Apply suggestions from code review

0f920c7

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Merge branch 'enh/get_data/tmin_tmax' of github.com:sappelhoff/mne-py…

2f306ab

…thon into enh/get_data/tmin_tmax

leftover items from code review

0bbb1c3

show proposed fix [ci skip]

14a70b9

add tmin/tmax for Evoked + tests [ci skip]

99fe7bd

larsoner reviewed Jul 15, 2021

View reviewed changes

mne/preprocessing/ecg.py Outdated Show resolved Hide resolved

sappelhoff added 8 commits July 15, 2021 16:49

raise if start/stop is int for Epochs/Evoked

bef7f59

add basic tests [ci skip]

a9c324a

Merge branch 'main' into enh/get_data/tmin_tmax

2131923

enh: add tmin/tmax to epochs.get_data()

1f5e9af

add tests

be0ab06

use single quotes, raw: validate tmin/max numeric, not int-like

992d9f7

fix test

8d0ee65

add whatsnew

61dc92c

sappelhoff changed the title ~~[WIP] Add tmin/tmax parameters to get_data methods~~ [MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg Jul 15, 2021

sappelhoff commented Jul 15, 2021

View reviewed changes

doc/changes/latest.inc Outdated Show resolved Hide resolved

larsoner reviewed Jul 15, 2021

View reviewed changes

mne/epochs.py Outdated Show resolved Hide resolved

sappelhoff added 3 commits July 16, 2021 13:09

Merge branch 'main' into enh/get_data/tmin_tmax

2295adb

Update doc/changes/latest.inc

9a052d4

Merge branch 'enh/get_data/tmin_tmax' of github.com:sappelhoff/mne-py…

e10095b

…thon into enh/get_data/tmin_tmax

fix and refactor

f02c9c9

larsoner merged commit 08b4b0e into mne-tools:main Jul 16, 2021

sappelhoff deleted the enh/get_data/tmin_tmax branch July 16, 2021 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg#9556

[MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg#9556
larsoner merged 24 commits intomne-tools:mainfrom
sappelhoff:enh/get_data/tmin_tmax

sappelhoff commented Jul 12, 2021 •

edited

Loading

Uh oh!

sappelhoff left a comment

Uh oh!

Uh oh!

sappelhoff commented Jul 13, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sappelhoff left a comment

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

sappelhoff commented Jul 16, 2021

Uh oh!

larsoner commented Jul 16, 2021

Uh oh!

hoechenberger commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

sappelhoff commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sappelhoff commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

sappelhoff commented Jul 16, 2021

Uh oh!

larsoner commented Jul 16, 2021

Uh oh!

hoechenberger commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sappelhoff commented Jul 12, 2021 •

edited

Loading

sappelhoff commented Jul 13, 2021 •

edited

Loading