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

[sappelhoff]

closes #9548

Work in progress

To do

• add to Raw
  • add tests
• add to Epochs
  • pending [MRG] Add units parameter to get_data for Epochs #9553 to avoid conflicts
  • add tests
• add to Evoked
  • pending Add get_data method to Evoked #9555 because until then Evoked has no get_data method
  • add tests
• fix find_bads_ecg: [MRG] ENH, FIX: Add tmin/tmax parameters to get_data methods, fix bug in find_bads_ecg #9556 (comment)
  • add tests
• add whatsnew (1) tmin/tmax, (2) fix of find_bads_ecg

[sappelhoff]

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

[sappelhoff]

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.

[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]

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

[sappelhoff]

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]

Thanks @sappelhoff !

[hoechenberger]

Amazing, thanks, @sappelhoff!