WIP FIX Deprecate use_rounding=False default in time_as_index [circle full] – take 2

[mathias-sm]

Reference issue (if any)

Implements the changes suggested in #13634 to estimate the impact of defaulting to truncation to defaulting to rounding when converting time to indices.

This is a copy / continuation of #13848 which I accidentally closed by moving my changes to branch fix-issue-13634-rounding and reverting my main to the upstream's main so that I could work on another PR. It'll teach me a lesson, sorry for the noise.

What does this implement/fix?

time_as_index defaulted to truncation (use_rounding=False), causing get_data(tmin=...) to return different samples from crop(tmin=...).get_data() at certain times due to floating-point truncation. Change the default to None and rounding. Emits a FutureWarning prompting callers to pass True or False explicitly. The goal of this commit is to estiate the impact of this change; follow-up commit should internally update all use of time_as_index to explicitely set a desired default.

Additional information

I have not yet updated all of the internal use of time_as_index, some of which explicitely use either use_rounding=False or use_rounding=False. Internall calls that do not pass a value for use_rounding will emit warnings. This is not ready for merging and is submitted as WIP / as a draft PR to trigger all tests and estimate the impact of this change.

What do we know so far?

From the previous discussion, we know that:

• In the existing tests, mne/io/egi/tests/test_egi.py::test_egi_mff_pause is the only test that breaks with this change: it boils down to onset/offsets comparisons that were hardcoded and are therefore now off-by-one.
• Temporarily, the Warnings are suppressed because those break tests. If we go ahead with these changes, I will have to either update the tests to catch those warnings or to set explicit use_rounding to avoid them in the first place.
• There are about 29 places in the codebase where time_as_index is called without explicitely setting use_rounding, which would need updating if we go ahead with this change.
• As far as I can tell nothing else was broken.

[welcome]

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

[larsoner]

To really see the impact I would suggest following my RuntimeError suggestion from #13634 (comment)

By computing both ways and raising this error it would clearly show all tests and examples (with a [circle full]) there the behavior would actually change.

[mathias-sm]

Ah yes indeed somehow I had forgotten about this. Quick exploration of that: it breaks 5 tests. I will push this after making sure that I'm doing things right to get them from the CI.

FAILED mne/io/tests/test_raw.py::test_get_data_tmin_tmax - RuntimeError: This would have returned a different value: index=array([600.61499023]), different from np.round(index)=array([601.]).
FAILED mne/stats/tests/test_erp.py::test_compute_sme - RuntimeError: This would have returned a different value: index=array([180.06149902]), different from np.round(index)=array([180.]).
FAILED mne/tests/test_evoked.py::test_get_peak - RuntimeError: This would have returned a different value: index=array([30.03074951]), different from np.round(index)=array([30.]).
FAILED mne/viz/tests/test_ica.py::test_plot_ica_overlay - RuntimeError: This would have returned a different value: index=array([1801.8449707]), different from np.round(index)=array([1802.]).
FAILED mne/tests/test_proj.py::test_compute_proj_raw - RuntimeError: This would have returned a different value: index=array([1501.53747559]), different from np.round(index)=array([1502.]).

[larsoner]

... and CIs won't run until the style runs are happy!

Thanks for working on this

[larsoner]

Looks like there are a number of failures:

= 56 failed, 6180 passed, 29 skipped, 109 deselected, 8 xfailed, 2 xpassed in 3609.83s (1:00:09) =

but a whole bunch are a single parametrization of mne/preprocessing/tests/test_realign.py, so it's really more like ~12 failures / tests that need to be updated. Want to try to fix those next?

[mathias-sm]

Yes, I'll give that a try!

[mathias-sm]

Just to make sure, before I start patching things here and there. Obviously one way to fix this is to explicitely pass use_rounding=False wherever the tests now fail (e.g. below). But if the goal is to learn something from this, then I guess maybe that kind of patching is not very useful.

--- a/mne/preprocessing/tests/test_realign.py
+++ b/mne/preprocessing/tests/test_realign.py
@@ -166,14 +166,14 @@ def _assert_boxcar_annot_similarity(raw, other):
     onsets_raw, dur_raw, onsets_other, dur_other = _annot_to_onset_dur(raw, other)

     n_events = len(onsets_raw)
-    onsets_samp_raw = raw.time_as_index(onsets_raw)
-    offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw)
+    onsets_samp_raw = raw.time_as_index(onsets_raw, use_rounding=False)
+    offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw, use_rounding=False)
     assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 2], [0] * n_events)
     assert_allclose(raw.get_data("stim")[0, onsets_samp_raw + 2], [1] * n_events)
     assert_allclose(raw.get_data("stim")[0, offsets_samp_raw - 2], [1] * n_events)
     assert_allclose(raw.get_data("stim")[0, offsets_samp_raw + 2], [0] * n_events)
-    onsets_samp_other = other.time_as_index(onsets_other)
-    offsets_samp_other = other.time_as_index(onsets_other + dur_other)
+    onsets_samp_other = other.time_as_index(onsets_other, use_rounding=False)
+    offsets_samp_other = other.time_as_index(onsets_other + dur_other, use_rounding=False)
     assert_allclose(other.get_data("stim")[0, onsets_samp_other - 2], [0] * n_events)
     assert_allclose(other.get_data("stim")[0, onsets_samp_other + 2], [1] * n_events)
     assert_allclose(other.get_data("stim")[0, offsets_samp_other - 2], [1] * n_events)

---

Inspectingmne/preprocessing/tests/test_realign.py::test_realign[0-0-0-0-0.9] is interesting: the test is as follows, on a synthetic 50s, 100Hz signal that has 500ms boxcars every second.

onsets_samp_raw = raw.time_as_index(onsets_raw)
offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw)
assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 2], [0] * n_events)
assert_allclose(raw.get_data("stim")[0, onsets_samp_raw + 2], [1] * n_events)

Here, raw.get_data("stim")[0] is such that: self.get_data("stim")[0,100*np.arange(50)] is basically just 1.s, and the sample before self.get_data("stim")[0,100*np.arange(2,48)-1] is basically just 0.s.

The output time_as_index without rounding (previous behavior) is: array([200, 300, 400, 500, 600, 699, 799, 900, ..]). I would consider indices 5 and 6 to be incorrect – however, the test used to pass because instead of confirming that the onsets-1 were all 0.s and the onsets were all 1.s, the current tests looks 2 samples before or after. So in my view, a better fix would be as follows:

--- a/mne/preprocessing/tests/test_realign.py
+++ b/mne/preprocessing/tests/test_realign.py
@@ -168,16 +168,16 @@ def _assert_boxcar_annot_similarity(raw, other):
     n_events = len(onsets_raw)
     onsets_samp_raw = raw.time_as_index(onsets_raw)
     offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw)
-    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 2], [0] * n_events)
-    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw + 2], [1] * n_events)
-    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw - 2], [1] * n_events)
-    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw + 2], [0] * n_events)
+    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 1], [0] * n_events)
+    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw], [1] * n_events)
+    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw - 1], [1] * n_events)
+    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw], [0] * n_events)
     onsets_samp_other = other.time_as_index(onsets_other)
     offsets_samp_other = other.time_as_index(onsets_other + dur_other)
-    assert_allclose(other.get_data("stim")[0, onsets_samp_other - 2], [0] * n_events)
-    assert_allclose(other.get_data("stim")[0, onsets_samp_other + 2], [1] * n_events)
-    assert_allclose(other.get_data("stim")[0, offsets_samp_other - 2], [1] * n_events)
-    assert_allclose(other.get_data("stim")[0, offsets_samp_other + 2], [0] * n_events)
+    assert_allclose(other.get_data("stim")[0, onsets_samp_other - 1], [0] * n_events)
+    assert_allclose(other.get_data("stim")[0, onsets_samp_other], [1] * n_events)
+    assert_allclose(other.get_data("stim")[0, offsets_samp_other - 1], [1] * n_events)
+    assert_allclose(other.get_data("stim")[0, offsets_samp_other], [0] * n_events)

This fixes this particular test variant. The raw.get_data part is also fixed for all other variants. But some of the other.get_data tests fail. My feeling here is that the previous bounds are too lenient and were letting through values that I would have considered incorrect. other is being resampled, so I can see why generous bounds make sense; raw on the other hand was passing a test despite an off-by one?

So yet another option would be yet another: keep the other.get_data bounds generous, but tighten raw.get_data ; and set use_rounding to True (i) match future default, and (2) to avoid it being None and triggering the comparison between the two version, which raises RuntimeError.

--- a/mne/preprocessing/tests/test_realign.py
+++ b/mne/preprocessing/tests/test_realign.py
@@ -166,14 +166,14 @@ def _assert_boxcar_annot_similarity(raw, other):
     onsets_raw, dur_raw, onsets_other, dur_other = _annot_to_onset_dur(raw, other)

     n_events = len(onsets_raw)
-    onsets_samp_raw = raw.time_as_index(onsets_raw)
-    offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw)
-    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 2], [0] * n_events)
-    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw + 2], [1] * n_events)
-    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw - 2], [1] * n_events)
-    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw + 2], [0] * n_events)
-    onsets_samp_other = other.time_as_index(onsets_other)
-    offsets_samp_other = other.time_as_index(onsets_other + dur_other)
+    onsets_samp_raw = raw.time_as_index(onsets_raw, use_rounding=True)
+    offsets_samp_raw = raw.time_as_index(onsets_raw + dur_raw, use_rounding=True)
+    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw - 1], [0] * n_events)
+    assert_allclose(raw.get_data("stim")[0, onsets_samp_raw], [1] * n_events)
+    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw - 1], [1] * n_events)
+    assert_allclose(raw.get_data("stim")[0, offsets_samp_raw], [0] * n_events)
+    onsets_samp_other = other.time_as_index(onsets_other, use_rounding=True)
+    offsets_samp_other = other.time_as_index(onsets_other + dur_other, use_rounding=True)
     assert_allclose(other.get_data("stim")[0, onsets_samp_other - 2], [0] * n_events)
     assert_allclose(other.get_data("stim")[0, onsets_samp_other + 2], [1] * n_events)
     assert_allclose(other.get_data("stim")[0, offsets_samp_other - 2], [1] * n_events)

---

Zooming out: I can keep digging through these and surfacing what I find. Should I do this as time goes by with messages here, or just implement what I think is best at the risk of not matching what the core team would think best?

[larsoner]

My feeling here is that the previous bounds are too lenient and were letting through values that I would have considered incorrect

Yeah that sounds right to me. Hopefully most tests can be reasoned out like this 🤞

Easiest way forward might actually be for you to implement the change that you think makes the most sense, and the n comment inline on GitHub on this PR saying why that change was made. Like you could have made the changes from "yet another option" above and then inline on the diff in GitHub you could have commented the "My feeling here is ..." ideas. Make sense / sound doable?

Thanks for looking into this