Fix copy-view issue in epochs

[pablomainar]

Closes #12114

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

[agramfort]

@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]

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]

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.

[pablomainar]

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

[drammock]

@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]

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]

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]

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.

[pablomainar]

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]

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]

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]

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]

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

[larsoner]

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:

1. 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.)
2. 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?

[drammock]

@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]

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]

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]

@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]

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!

[larsoner]

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]

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

[sappelhoff]

@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]

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