Fix: Rounding error in get_data with tmin/tmax

[git-gigi]

Description

This PR fixes an off-by-one inconsistency between epochs.crop() and epochs.get_data() caused by floating point truncation when converting time to indices.

Analysis

Previously, _handle_tmin_tmax used the default behavior of time_as_index (which performs a floor operation/truncation). When passing a float time t (e.g., 0.77) that is represented internally as slightly less (e.g., 0.76999...), get_data(tmin=t) would return the sample at 0.76 instead of 0.77.
crop() correctly uses use_rounding=True, leading to inconsistent results for the same input time.

Fix

• Added use_rounding=True to time_as_index calls inside mne/utils/mixin.py.
• Added a regression test in mne/tests/test_epochs.py reproducing the issue reported in In crop(tmin) versus get_data(tmin), tmin has a different meaning #13634.

Closes #13634

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

[drammock]

adding a new private method to Epochs that has the same name as an existing utility function is not the right way to go about this. It duplicates code and introduces the possibility of epochs behaving differently than Raw/Evoked for example.

[git-gigi]

Thanks for the review @drammock.

I understand the concern about code duplication. I initially tried modifying the shared _handle_tmin_tmax in mixin.py, but adding use_rounding=True there caused regressions in Raw tests (shifting data by one sample on Windows environments due to float precision). That's why I attempted the override in Epochs.

The core issue linked (#13634) is that epochs.crop(tmin=t) includes a sample that epochs.get_data(tmin=t) excludes. Since crop uses rounding internally, users expect get_data to match that behavior for consistency.

If modifying the global mixin.py is risky for Raw backward compatibility, and overriding in Epochs is discouraged, do you have a suggestion on how to reconcile get_data with crop for Epochs specifically? Maybe passing a round_tmin argument to get_data?

[drammock]

The core issue linked (#13634) is that epochs.crop(tmin=t) includes a sample that epochs.get_data(tmin=t) excludes. Since crop uses rounding internally, users expect get_data to match that behavior for consistency.

yes, that was clear from the issue description.

If modifying the global mixin.py is risky for Raw backward compatibility, and overriding in Epochs is discouraged, do you have a suggestion on how to reconcile get_data with crop for Epochs specifically? Maybe passing a round_tmin argument to get_data?

I suspect that the problem is not limited to Epochs, but also affects Raw and Evoked (and TFR... anything that inherits the mixin). I don't have a suggestion off the top of my head; as I said before, this will need some discussion. Changing how get_data() works (to accord with crop()) --- or vice-versa --- has potentially wide-reaching consequences. I know it's a single sample, but as you've seen it's enough to break our tests, and it's also enough to change the results of user's existing analysis code, or even cause that code to crash if re-run. We don't take that lightly.

[drammock]

This will need some discussion. When use_rounding was introduced, it was determined that we shouldn't change the default:

#2311 (comment)

[drammock]

code comments in English please. But also: code comments should be more specific/useful than "here is the fix for Epochs". This one isn't really needed at all IMO

Suggested change

# QUI c'è la fix specifica per le Epochs

[git-gigi]

Sorry about that! I left a debug comment by mistake. I will remove it in the next commit

[git-gigi]

Thanks @drammock. I get why this is tricky regarding backward compatibility. I'll stop working on the code for now and wait for the discussion. Just let me know if you want me to check Raw or Evoked in the meantime - happy to help if needed :)