mne-python
mne-python copied to clipboard
mne-tools
Adding an argument `include_tmax` for `.get_data()`
We ran into a failure today in https://github.com/vferat/pycrostates/pull/14 because raw.get_data() was returning one less sample than expected, namely, the tmax sample.
from pathlib import Path
import mne
print (mne.__version__)
directory = Path(mne.datasets.sample.data_path()) / 'MEG' / 'sample'
fname = directory / 'sample_audvis_filt-0-40_raw.fif'
raw = mne.io.read_raw_fif(fname, preload=True, verbose=False)
print (raw.get_data().shape)
print (raw.get_data(tmin=0, tmax=raw.times[-1]).shape)
OUT:
0.24.0
(376, 41700)
(376, 41699)
Do you consider this to be a bug in the handling of tmax, or should an additional argument be added, include_tmax, like for raw.crop()?
I don't think this is a bug either, but a convention. Consider a signal sampled with 250 Hz, and you would like to get exactly one second of data (let's say the first second for simplicity). Which samples do you expect to get? I'd say I should get the first 250 samples corresponding to time 0 and 1 – 1/250 s (so the last sample at time 1 s is excluded).
Python indexing works like that too, so if you want the last sample I'd use tmax=None such that raw.get_data(tmin=0) gives the correct result. I don't think we need a new parameter.
I actually never thought about this in terms of python indexing, that does help a lot!
Sure there is a way to get by with the current arguments; but I feel like as for .crop(), the fact that an argument include_tmax exists makes it clearer for the user that by default, the tmax is not included.
Also, the docstring for stop and tmax could be improved a bit. For tmax, it doesn't say that it's excluded; and you have to look at stop to understand that tmax behaves in the same way and corresponds to the first time not to include.
Since we have include_tmax for crop, +1 for adding it here for consistency
... might also be relevant for TFR objects after https://github.com/mne-tools/mne-python/pull/10945