WIP FIX Deprecate use_rounding=False default in time_as_index [circle full] – take 2#13907
WIP FIX Deprecate use_rounding=False default in time_as_index [circle full] – take 2#13907mathias-sm wants to merge 16 commits into
Conversation
`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.
The goal here is to measure the impact of changing the default value of `use_rounding=False` in time_as_index`, with a FutureWarning deprectation step to warn users. Unfortunately the tests fail hard on seeing said warning so most tests fail but it's not because the underlying behavior has changed, it's because of the warning. This commit removes the warning to see where is the change in behavior surfacing actuall problems.
for more information, see https://pre-commit.ci
Again, in order to check the impact of changing the default value of `use_rounding` in `time_as_index` from False to True, I am providing a (possibly temporary) fix to `mne/io/egi/tests/test_egi.py::test_egi_mff_pause` so that it passes and we can see if there are other downstream issues.
for more information, see https://pre-commit.ci
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
| # Turned off temporarily to see the impact of the change without | ||
| # crashing on a FutureWarning |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.]).
c95171e to
b712a6a
Compare
for more information, see https://pre-commit.ci
|
... and CIs won't run until the style runs are happy! Thanks for working on this |
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
for more information, see https://pre-commit.ci
|
Looks like there are a number of failures: but a whole bunch are a single parametrization of |
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-roundingand reverting mymainto the upstream'smainso that I could work on another PR. It'll teach me a lesson, sorry for the noise.What does this implement/fix?
time_as_indexdefaulted to truncation (use_rounding=False), causingget_data(tmin=...)to return different samples fromcrop(tmin=...).get_data()at certain times due to floating-point truncation. Change the default toNoneand rounding. Emits aFutureWarningprompting callers to passTrueorFalseexplicitly. The goal of this commit is to estiate the impact of this change; follow-up commit should internally update all use oftime_as_indexto 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 eitheruse_rounding=Falseoruse_rounding=False. Internall calls that do not pass a value foruse_roundingwill 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:
mne/io/egi/tests/test_egi.py::test_egi_mff_pauseis 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.use_roundingto avoid them in the first place.