Fix copy-view issue in epochs #12121

pablomainar · 2023-10-21T17:50:22Z

Add deprecation
Get pytest runs green
Get [circle full] green

for more information, see https://pre-commit.ci

agramfort · 2023-10-23T10:54:55Z

@pablomainar thx for the PR. you'll need to add tests. thx

drammock

Is this supposed to be different from #12117 in some way? Trying to figure out why that one was closed and this one opened in its place... the diff looks the same though.

drammock · 2023-10-23T14:11:16Z

mne/epochs.py

+        if copy:
+            data = data.copy()


I suggest moving this up about 25 lines, inside the if branch of the if self.preload conditional. The else branch defines data = np.empty(...) so that will never be a view of the object's data and thus the copy will never be necessary.

drammock · 2023-10-23T14:14:05Z

mne/epochs.py

@@ -1588,6 +1588,7 @@ def _get_data(
        units=None,
        tmin=None,
        tmax=None,
+        copy=True,


in this private method we should probably default to copy=False so that we're not making unnecessary copies in internal calls that use the private API.

mne/epochs.py

pablomainar · 2023-10-25T11:52:55Z

@drammock thanks for the suggestions, I updated the PR accordingly

drammock · 2023-10-30T20:24:02Z

@pablomainar there are several failing CIs. Can you look into them? LMK if you need help or guidance. Here's a link to two failures that look related: https://github.com/mne-tools/mne-python/actions/runs/6639926213/job/18137375032?pr=12121#step:17:4209

I recommend running those two tests locally and seeing if you can figure out what is causing the failure. pytest mne/tests/test_epochs.py -k baseline_basic and pytest mne/viz/tests/test_epochs.py -k plot_psd_epochs

pablomainar · 2023-11-02T07:20:14Z

Done. I changed the test slightly because the two failing ones were assuming that a view was returned from get_data(), which is no longer the case by default. Should I also write other tests to check that a copy is returned instead of a view when the copy argument is set to True?

drammock · 2023-11-02T19:40:45Z

Should I also write other tests to check that a copy is returned instead of a view when the copy argument is set to True?

yes, a small test to verify that a copy is returned would be good. Using np.array.base would be a good way to test it: https://numpy.org/doc/stable/reference/generated/numpy.ndarray.base.html

drammock · 2023-11-02T19:41:41Z

also heads-up that I merged in main here to get the tests to run (and pass, there were some CI failures here that should be fixed on main). So be sure to git pull before working on the new tests.

for more information, see https://pre-commit.ci

pablomainar · 2023-11-02T20:50:32Z

Should I also write other tests to check that a copy is returned instead of a view when the copy argument is set to True?

yes, a small test to verify that a copy is returned would be good. Using np.array.base would be a good way to test it: https://numpy.org/doc/stable/reference/generated/numpy.ndarray.base.html

Well, I'm not sure if using base would work because get_data(copy=True) returns a view of a copy of the underlying _data. Therefore calling base on it, will return an array, not a None value.
Instead, I have modified the copy and then called again get_data() to make sure that result has not been modified. I have also added a second test for get_data(copy=False) with the same structure. I am not sure if this second test is needed, specially because it is changing the underlying _data and therefore corrupting the epochs. Might be confusing for future tests. Let me know what you think :)

drammock · 2023-11-02T21:57:49Z

I'm not sure if using base would work because get_data(copy=True) returns a view of a copy of the underlying _data.

Ah OK, that's because we always do data[select] at some point right? I wonder how important it is for data.base and data.flags.owndata to look "correct" here. @larsoner do you think it's a problem that epochs.get_data(copy=True).flags.owndata will always be (unexpectedly) False if epochs is preloaded? Right now on this PR .flags.owndata is 100% determined by whether the data are preloaded (assuming no picks passed):

import mne

sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = sample_data_folder / "MEG" / "sample" / "sample_audvis_raw.fif"
raw = mne.io.read_raw_fif(sample_data_raw_file, verbose=False, preload=False)
events = mne.find_events(raw, stim_channel="STI 014")
event_dict = {"auditory/left": 1}
kwargs = dict(event_id=event_dict, proj=False)
ondisk_epochs = mne.Epochs(raw, events, preload=False, **kwargs)
preloaded_epochs = mne.Epochs(raw, events, preload=True, **kwargs)

print(
    preloaded_epochs.get_data(copy=False).flags.owndata,
    preloaded_epochs.get_data(copy=True).flags.owndata,
)
# False False

print(
    ondisk_epochs.get_data(copy=False).flags.owndata,
    ondisk_epochs.get_data(copy=True).flags.owndata,
)
# True True

Ideally for preloaded epochs we should get owndata == False when copy=False and owndata == True when copy=True (again, as long as no picks are passed)

larsoner · 2023-11-03T00:00:42Z

Instead of checking owndata directly in the preloaded case we can use np.shares_memory(out_data, epochs._data). It should be True for copy=False and False for copy=True. np.may_share_memory probably would also work and be faster

https://numpy.org/doc/stable/reference/generated/numpy.may_share_memory.html#numpy.may_share_memory

pablomainar · 2023-11-03T08:21:44Z

Good idea @larsoner, I changed the test to use np.shares_memory. Because it is just a test with a small dataset, it won't be much slower than np.may_share_memory.
However the issue raised by @drammock is still valid: a user might be confused if they want to check if data is copy and data.flags.owndata equals False. Is this a problem?

larsoner · 2023-11-03T12:08:36Z

I think that's a pretty advanced use case / user who would do this so it's okay

larsoner · 2023-11-03T13:09:16Z

FYI I'm going to merge in main to get all tests to run. For the future if you click the CircleCI stotus / Details and log into GitHub there it will let those build on your PRs

larsoner

Last we need a changelog entry. We have two options:

Make the default copy=None which means "use copy=False and if it matters (epochs preloaded and no fancy indexing so it's a view), emit a warning that the default will change to copy=True in 1.6". This is the safest but most annoying option to code. Then you would add an entry in the API section because it's a deprecation. I think I'm +0.5 on this one. (I can help with this step with more explanation or a quick commit if it's not clear.)
Make the default copy=True now, which you've done. This is the simplest, and would then require an entry in the Bug section saying that this kwarg was added with default copy=True to avoid silent, possibly errant / dangerous behavior from 1.5.

@drammock any opinion on which we should do?

mne/epochs.py

drammock · 2023-11-03T16:47:35Z

@drammock any opinion on which we should do?

I vote for option 2. I still think people should not have been relying on the fact that we returned a view, so if those (presumably rare) cases break I'm OK with it. It's outweighed by the potential good of eliminating silent bugs. see below

cbrnr · 2023-11-03T16:52:24Z

This sounds a bit bold in the light of many previous discussions on deprecations and not breaking people's code, given that this behavior is (was) clearly documented:

Returns
data : numpy.ndarray of shape (n_epochs, n_channels, n_times)
A view on epochs data.

Why do you now say "people should not have been relying on the fact that we returned a view"?

drammock · 2023-11-03T16:53:39Z

This sounds a bit bold in the light of many previous discussions on deprecations and not breaking people's code, given that this behavior is (was) clearly documented:

argh, you're right, I forgot (again) that this was documented behavior. Sorry. I switch my vote to doing a proper deprecation.

larsoner · 2023-11-07T19:01:25Z

@pablomainar do you need help with the deprecation? If so I can push some quick commits. We'll probably release 1.6 in the next week or so and it would be great to get this in!

pablomainar · 2023-11-08T14:14:56Z

Sorry @larsoner, I've been quite busy this week. I'm not sure I'll find time to finish this in the next week, it would be very helpful if you could help me out. Thanks a lot!

* upstream/main: BUG: Fix bug with spectrum warning (mne-tools#12186) Add argument splash to disable splash-screen from Qt-browser (mne-tools#12185) BUG: Fix bug with logging and n_jobs>1 (mne-tools#12154) Use gray logo (works in light and dark modes) (mne-tools#12184) Tweak logo for dark mode (mne-tools#12176) ENH: Improve Covariance.__repr__ (mne-tools#12181) ENH: Enable sensor-specific OPM coregistration in mne coreg (mne-tools#11405) Tweak README.rst (mne-tools#12166) [pre-commit.ci] pre-commit autoupdate (mne-tools#12177) MAINT: Add branch coverage (mne-tools#12174) OpenSSF (mne-tools#12175) fix docstring in 60_sleep.py (mne-tools#12171) FIX: skip empty lines in read_raw_eyelink (mne-tools#12172) FIX: Fix bug with coreg scalars (mne-tools#12164) Changed casting rule in np.clip to allow reading of raw GDF files (mne-tools#12168) [DOC] Add documentation for setting montage order (mne-tools#12160) Fix inferring fiducials from EEGLAB (mne-tools#12165)

* upstream/main: MAINT: Fix CIs (mne-tools#12188) BUG: Fix bug with default alpha and axes (mne-tools#12187)

larsoner · 2023-11-09T21:06:58Z

Deprecation complete, ready for review/merge @drammock

I ran [circle full] earlier and modified all the failing examples in this PR so we should be good once this lands in main

welcome · 2023-11-09T21:51:57Z

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

…o-pyproject.toml * upstream/main: Fix copy-view issue in epochs (mne-tools#12121)

sappelhoff · 2023-11-14T10:04:25Z

@drammock, tagging you as you seem to have stumbled over my issue in mne-bids (and autoreject) --> shouldn't the new "copy" parameter for the Epochs' get_data method have been marked with "New in v1.6"?

larsoner · 2023-11-14T14:32:56Z

Yes it should have that, too, I'll add it in the soon-to-be-merged #12178

* MAINT: Update documentation and logging - Nice new landing page - Added covenience makefile commands - improved logging of flagged epochs * FIX, MAINT: Bump iclabel version and ignore mne warning We already throw an error if the user has mne iclabel version less than .5, so added a pin for minimum version of .5 in the requirements file. MNE is also throwing a future warnign for an API kwarg change in epochs.get_data, where the current default copy=False is changing to copy=True. See mne-tools/mne-python#12121 . I'm a little worried about the additional copies we will incur every single time that we call that method, but I dont have a strong reason to stray from what MNE believes is a bug fix so lets go with it and see how it plays. * FIX, MAINT: Bumpg GH actions checkout versions - This should resolve the core issue of the runner not being able to find the most up to date mne_iclabel wheel. * FIX, TST: Install torch in doc building CI Since MNE iclabel 0.5, torch is NOT installed by default.

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel McCloy <[email protected]> Co-authored-by: Eric Larson <[email protected]>

Fix copy-view issue in epochs

5bd0dac

pablomainar requested review from drammock, agramfort, mscheltienne and dengemann as code owners October 21, 2023 17:50

[pre-commit.ci] auto fixes from pre-commit.com hooks

f0f7173

for more information, see https://pre-commit.ci

drammock reviewed Oct 23, 2023

View reviewed changes

Add PR suggestions

15816db

Adapt tests to new interface

98b01b6

Merge branch 'main' into fix

19a30da

pablomainar and others added 2 commits November 2, 2023 21:45

Add tests for copy

6436a68

[pre-commit.ci] auto fixes from pre-commit.com hooks

d740c31

for more information, see https://pre-commit.ci

Use shares_memory in test

1346645

Merge branch 'main' into fix

d310732

larsoner reviewed Nov 3, 2023

View reviewed changes

mne/epochs.py Outdated Show resolved Hide resolved

drammock reviewed Nov 3, 2023

View reviewed changes

mne/epochs.py Outdated Show resolved Hide resolved

larsoner added this to the 1.6 milestone Nov 7, 2023

larsoner added 2 commits November 8, 2023 13:27

FIX: Close

69ca518

larsoner requested review from adam2392, cbrnr, jasmainak, britta-wstnr and wmvanvliet as code owners November 8, 2023 18:28

larsoner added 5 commits November 8, 2023 13:49

FIX: More

a0c7ade

FIX: Green [circle full]

09792b5

Merge remote-tracking branch 'upstream/main' into fix

1135dd9

* upstream/main: MAINT: Fix CIs (mne-tools#12188) BUG: Fix bug with default alpha and axes (mne-tools#12187)

FIX: Examples

01e1d9c

FIX: Simpler

b2e7c46

drammock approved these changes Nov 9, 2023

View reviewed changes

drammock merged commit 7b3e3c9 into mne-tools:main Nov 9, 2023
28 checks passed

larsoner added a commit to hoechenberger/mne-python that referenced this pull request Nov 13, 2023

Merge remote-tracking branch 'upstream/main' into move-requirements-t…

2c50bfe

…o-pyproject.toml * upstream/main: Fix copy-view issue in epochs (mne-tools#12121)

drammock mentioned this pull request Nov 13, 2023

FutureWarning: with copy=False --> set default explicity mne-tools/mne-bids#1190

Closed

This was referenced Nov 18, 2023

[MAINT] Refactor bivariate and multivariate methods into separate files mne-tools/mne-connectivity#156

Merged

[MAINT] Fix failing unit tests & update CI packages mne-tools/mne-connectivity#157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copy-view issue in epochs #12121

Fix copy-view issue in epochs #12121

pablomainar commented Oct 21, 2023 •

edited by hoechenberger

Loading

agramfort commented Oct 23, 2023

drammock left a comment

drammock Oct 23, 2023

drammock Oct 23, 2023

pablomainar commented Oct 25, 2023

drammock commented Oct 30, 2023 •

edited

Loading

pablomainar commented Nov 2, 2023

drammock commented Nov 2, 2023

drammock commented Nov 2, 2023

pablomainar commented Nov 2, 2023

drammock commented Nov 2, 2023

larsoner commented Nov 3, 2023 •

edited

Loading

pablomainar commented Nov 3, 2023

larsoner commented Nov 3, 2023

larsoner commented Nov 3, 2023

larsoner left a comment

drammock commented Nov 3, 2023 •

edited

Loading

cbrnr commented Nov 3, 2023

drammock commented Nov 3, 2023

larsoner commented Nov 7, 2023

pablomainar commented Nov 8, 2023

larsoner commented Nov 9, 2023

welcome bot commented Nov 9, 2023

sappelhoff commented Nov 14, 2023

larsoner commented Nov 14, 2023

Fix copy-view issue in epochs #12121

Fix copy-view issue in epochs #12121

Conversation

pablomainar commented Oct 21, 2023 • edited by hoechenberger Loading

agramfort commented Oct 23, 2023

drammock left a comment

Choose a reason for hiding this comment

drammock Oct 23, 2023

Choose a reason for hiding this comment

drammock Oct 23, 2023

Choose a reason for hiding this comment

pablomainar commented Oct 25, 2023

drammock commented Oct 30, 2023 • edited Loading

pablomainar commented Nov 2, 2023

drammock commented Nov 2, 2023

drammock commented Nov 2, 2023

pablomainar commented Nov 2, 2023

drammock commented Nov 2, 2023

larsoner commented Nov 3, 2023 • edited Loading

pablomainar commented Nov 3, 2023

larsoner commented Nov 3, 2023

larsoner commented Nov 3, 2023

larsoner left a comment

Choose a reason for hiding this comment

drammock commented Nov 3, 2023 • edited Loading

cbrnr commented Nov 3, 2023

drammock commented Nov 3, 2023

larsoner commented Nov 7, 2023

pablomainar commented Nov 8, 2023

larsoner commented Nov 9, 2023

welcome bot commented Nov 9, 2023

sappelhoff commented Nov 14, 2023

larsoner commented Nov 14, 2023

pablomainar commented Oct 21, 2023 •

edited by hoechenberger

Loading

drammock commented Oct 30, 2023 •

edited

Loading

larsoner commented Nov 3, 2023 •

edited

Loading

drammock commented Nov 3, 2023 •

edited

Loading