Describe the problem
The function mne.Epochs(raw, events, event_id, tmin=0.0, tmax=1.0) with a sampling rate at 500 Hz loads epochs with 501 points, which actually covers the time window [0.0, 1.002] (i.e. 501 x 1/500 = 1.002). I would expect mne.Epochs to take 500 time points to cover [0.0, 1.0]. Arguably, the time point at 1.0 is a limit case because it is the time where a new sample is taken, but even more buggy, in my opinion, is that tmax=0.999 still gives 501 time samples. We get 500 times points with tmax=0.9989.
I see two defendable positions on this topic:
1 - A sample time is taken to be the value of a signal at the middle of a sampling period (I think it is more theoretically sound)
2 - A sample time is taken to be the beginning of a sampling period (I think it is more practical and that it is what is generally implicitly considered)
A figure can clarify the issue. Consider:
In this figure, a) and b) illustrate the positions 1 and 2, respectively. The returned epoch is shown in yellow. Panels a) and b) correspond to the current behavior of MNE. Panel c) show the behavior of EEGLAB. If MNE takes the position to be inclusive on tmax, the returned epoch in a) and b) are consistent regardless on whether you take position 1 (panel a) or 2 (panel b). EEGLAB would be said to be non-inclusive on tmax.
Now, consider what happens for tmax=0.999. Positions 1 and 2 applied to MNE behavior are shown in panels d) and e). The behavior of EEGLAB is shown in panel f). Again, for position 1 (panel d), considering tmax inclusive, we are at the limit case and, although a weird result to me, MNE behavior is coherent. If we were to take position 2 (panel e), then MNE behavior would be wrong. f) shows the behavior of EEGLAB.
So, based on that, it seems to me that the current behavior of MNE is only compatible with 1) sampling point at the middle of the sampling period and 2) an inclusive tmax.
Describe your solution
To allow compatibility with other libraries like EEGLAB, I propose adding "inclusive=True" to the signature of mne.Epochs.
- With inclusive=True, the behavior would be as it currently is.
- With inclusive=False, the behavior would be such that samples are included if and only if tmin <= t_sample < t_max. This behavior would be compatible with other libraries such as EEGLAB.
Describe possible alternatives
This proposed solution is to avoid non-backward-compatible changes. Note that, as mentioned before, this works only if we consider the sampling point as being in the middle of the sampling period. That means that conversion from samples to time is such that t_sample == (no_sample+0.5)/sampling_rate with no_sample being indexed from 0. Note however that this is not what is generally considered. Most people would rather do no_sample/sampling_rate... and that is what is currently done elsewhere in MNE like at line 258 of https://github.com/mne-tools/mne-python/blob/master/mne/io/base.py#L265-L1836 : index = (np.atleast_1d(times) - self.times[0]) * sfreq. If we rather want to consider that t_sample = no_sample/sampling_rate, then the behavior of MNE is plainly erroneous for the example given with tmax=0.999 (panel e) and the solution to correct that would require non-backward-compatible changes.
Describe the problem
The function mne.Epochs(raw, events, event_id, tmin=0.0, tmax=1.0) with a sampling rate at 500 Hz loads epochs with 501 points, which actually covers the time window [0.0, 1.002] (i.e. 501 x 1/500 = 1.002). I would expect mne.Epochs to take 500 time points to cover [0.0, 1.0]. Arguably, the time point at 1.0 is a limit case because it is the time where a new sample is taken, but even more buggy, in my opinion, is that tmax=0.999 still gives 501 time samples. We get 500 times points with tmax=0.9989.
I see two defendable positions on this topic:
1 - A sample time is taken to be the value of a signal at the middle of a sampling period (I think it is more theoretically sound)
2 - A sample time is taken to be the beginning of a sampling period (I think it is more practical and that it is what is generally implicitly considered)
A figure can clarify the issue. Consider:
In this figure, a) and b) illustrate the positions 1 and 2, respectively. The returned epoch is shown in yellow. Panels a) and b) correspond to the current behavior of MNE. Panel c) show the behavior of EEGLAB. If MNE takes the position to be inclusive on tmax, the returned epoch in a) and b) are consistent regardless on whether you take position 1 (panel a) or 2 (panel b). EEGLAB would be said to be non-inclusive on tmax.
Now, consider what happens for tmax=0.999. Positions 1 and 2 applied to MNE behavior are shown in panels d) and e). The behavior of EEGLAB is shown in panel f). Again, for position 1 (panel d), considering tmax inclusive, we are at the limit case and, although a weird result to me, MNE behavior is coherent. If we were to take position 2 (panel e), then MNE behavior would be wrong. f) shows the behavior of EEGLAB.
So, based on that, it seems to me that the current behavior of MNE is only compatible with 1) sampling point at the middle of the sampling period and 2) an inclusive tmax.
Describe your solution
To allow compatibility with other libraries like EEGLAB, I propose adding "inclusive=True" to the signature of mne.Epochs.
Describe possible alternatives
This proposed solution is to avoid non-backward-compatible changes. Note that, as mentioned before, this works only if we consider the sampling point as being in the middle of the sampling period. That means that conversion from samples to time is such that t_sample == (no_sample+0.5)/sampling_rate with no_sample being indexed from 0. Note however that this is not what is generally considered. Most people would rather do no_sample/sampling_rate... and that is what is currently done elsewhere in MNE like at line 258 of https://github.com/mne-tools/mne-python/blob/master/mne/io/base.py#L265-L1836 :
index = (np.atleast_1d(times) - self.times[0]) * sfreq. If we rather want to consider that t_sample = no_sample/sampling_rate, then the behavior of MNE is plainly erroneous for the example given with tmax=0.999 (panel e) and the solution to correct that would require non-backward-compatible changes.