mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.69k stars 1.31k forks source link

BUG? Rounding time points instead of flooring them #1374

Closed rgoj closed 10 years ago

rgoj commented 10 years ago

This code in several places in epochs.py implicitly floors time points:

self.first = int(tmin * info['sfreq'])

Instead of doing rounding:

self.first = int(round(tmin * info['sfreq']))

This leads to issues like a time point -0.19979521 (200ms) getting conerted to -39 and then to -0.195 instead of the previous value or, at least, -0.2 (which lead to beamformer test errors in #1365)

I'm not sure how big an issue this is or how much impact it would have on all the code.

First mentioned in #1371

larsoner commented 10 years ago

Hmmm... I think our convention thus far has been to use integer truncation everywhere. It probably would have been better from the start to use floor, ceil, or round as opposed to truncation, but for backward compatibility we might need to continue using integer truncation. I don't have a problem with it so long as we make it clear to users that's how times are converted to index numbers.

rgoj commented 10 years ago

@Eric89GXL I'm OK with that. The truncation was a problem because the averaging method changed the time points quite significantly in the beamformer tests, but since that is now corrected (hacked?) with:

evoked.times = self.times

within _compute_mean_or_stderr that problem is solved. I'm not sure if it's relevant elsewhere, though, hence the issue.

larsoner commented 10 years ago

That line probably needs to be self.times.copy() (it's an ndarray, right?) to avoid writing one changing the data in another.

I think we'll just need to be careful about how we construct and access the arrays to ensure we're taking into account the integer truncation.

agramfort commented 10 years ago

+1 .copy()