You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Inconsistent behavior between Epochs and Evoked in get_data() method when picking EEG channels and bads are present #12577
changed the title [-]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method[/-][+]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method when picking[/+]on Apr 26, 2024
changed the title [-]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method when picking[/-][+]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels[/+]on Apr 26, 2024
changed the title [-]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels[/-][+]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels and bads are present[/+]on Apr 26, 2024
So something about this point is inconsistent between epochs and evoked at least when passing picks="eeg".
Neither get_data has an exclude param, sounds like one is implicitly using exclude="bads" and the other exclude=() under the hood. To change the behavior of one to match the other and be consistent I think we'd need a deprecation cycle, not sure whether or not it's worth the code churn. To update the docstring of both to reflect their actual behavior it wouldn't require a deprecation cycle. Adding exclude to both (to make the behavior more explicit and controllable) is probably worthwhile either way.
Even if documented I think it would be a bit surprising if Epochs and Evoked used different pick defaults. So I'd document the behavior with whatever version we think makes more sense, be consistent for both methods and deprecate the inconsistent behavior. And yes, an explicit exclude parameter would definitely make sense to me.
My feeling is that we should add exclude parameters to all these get_data() methods if they don't have one already, retaining the current behavior; and do a deprecation cycle for transitioning to either exclude=() or exclude="bads" for all of them
Me and two friends (@luisademelo14@Fooxfeir) are taking a course and we need to choose a tool and bug to fix. We chose MNE-Python, but since we are new to the community, we decided to pick a bug marked as easy and this one seems good.
If there's a problem with that, please let us know!
Reacted by Luisa de Melo Barros Penze and Fooxfeir
FYI not all of this issue is easy... the first part is probably uncontroversial: namely adding exclude params to functions that don't have them, retaining old behavior, and documenting things properly (#12197). I think documenting current behavior and making it changeable by adding exclude=... in places we don't have it is already a good start here, so @RenanFlorencio@luisademelo14@Fooxfeir I think you can work on this part! 👍 So feel free to start on that
The deprecation cycle to transition them all to have the same behavior will not be trivial. I suspect a lot of tests will need to be adjusted, and it will be hard to do without a reasonably deep working knowledge of the code.
I am also not 100% sure we should do the deprecation to make everything have either exclude=() or exclude="bads everywhere. There are probably functions and classes where we should exclude bads -- like in multivariate connectivity fitting for example we've talked about it and there are probably other examples -- and other functions/classes (like constructing epochs maybe) it is convenient to include them by default. We'd have to look at all use cases (decoding, inverses, stats, time-freq/spectra, connectivity, etc.) to see what makes sense here. Moreover, if we change the behavior of something like Epochs the risk of breaking people's code is quite high even with a deprecation cycle... so we should think really hard about whether the potential disruption is worth the uniformity, and how much it buys us.
Okay @larsoner , we worked on the first part and we added the exclude parameter to all three functions (epochs, evoked and base, since spectrum and tfr already have it), this way we have three different behaviors for each. We would like you to confirm this is what's expected to write the docstring. We also want to ask you a question that is at the end of this comment.
Here you can see that we changed the line inside the else block to add the exclude parameter. However, the if block is still "hard coded" to exclude=(). Should it also use the new exclude parameter? If so, we would have to figure out a way to keep the old behavior, since the else block would need a default exclude=bads, which we are using, and the if block would need exclude=().
Conclusion
There are two functions, in raw and evoked, use default exclude=(). The other three, in epochs, spectrum and tfr use default `exclude='bads'. If we were to do a deprecation cycle, we would have to think more about which one makes more sense. Moreover, do you have any suggestions of issues we could work on?
For this, rather than using print how about you write some tiny test on main that should pass that asserts something about shapes of returned data?
Then you add the exclude param defaults, and your test ensures that you haven't changed behavior.
Then you append to that same test some calls with different exclude values and assert something new/different about the obtained shapes.
This is a variant of test-driven development (strictly speaking there you'd write the whole test first even though it would fail, then modify code, but in practice it's nice to see stuff green / passing to start)
Lastly, we would like to ask you about the following lines of code in epochs.py at the _get_data function:
I would try not to mess with this one. Internal functions get split to handle specific use cases... the epochs code gets used all over the place so at the end of the day having CIs/tests green should tell us if things are okay or not.
If we were to do a deprecation cycle, we would have to think more about which one makes more sense.
Let's not do that for now... I think it would be more end-user pain that it's worth.
Moreover, do you have any suggestions of issues we could work on?
Anything with the EASY label should in principle be somewhat easy, at least that's the goal 😄
Activity
cbrnr commentedon Apr 26, 2024
The docstrings are identical, so I think both methods should behave identically. In addition, the docstring says:
I think this should be:
At least that's how it currently works for
Epochs, but not forEvokedapparently.https://mne.tools/dev/generated/mne.Epochs.html#mne.Epochs.get_data
https://mne.tools/dev/generated/mne.Evoked.html#mne.Evoked.get_data
hoechenberger commentedon Apr 26, 2024
@cbrnr Note that I did not pass
Nonein my MWE! I did passpicks="eeg"The doctoring says:
So something about this point is inconsistent between epochs and evoked at least when passing
picks="eeg".I think what you're mentioning here is being tracked at #12197
[-]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method[/-][+]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method when picking[/+][-]Inconsistent picking behavior between `Epochs` and `Evoked` in `get_data()` method when picking[/-][+]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels[/+][-]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels[/-][+]Inconsistent behavior between `Epochs` and `Evoked` in `get_data()` method when picking EEG channels and bads are present[/+]larsoner commentedon Apr 26, 2024
Neither
get_datahas anexcludeparam, sounds like one is implicitly usingexclude="bads"and the otherexclude=()under the hood. To change the behavior of one to match the other and be consistent I think we'd need a deprecation cycle, not sure whether or not it's worth the code churn. To update the docstring of both to reflect their actual behavior it wouldn't require a deprecation cycle. Addingexcludeto both (to make the behavior more explicit and controllable) is probably worthwhile either way.cbrnr commentedon Apr 26, 2024
Even if documented I think it would be a bit surprising if
EpochsandEvokedused different pick defaults. So I'd document the behavior with whatever version we think makes more sense, be consistent for both methods and deprecate the inconsistent behavior. And yes, an explicitexcludeparameter would definitely make sense to me.hoechenberger commentedon Apr 26, 2024
I tested the MWE with Raw and now it seems it's actually Epochs who's the odd one:
hoechenberger commentedon Apr 26, 2024
More digging:
BaseSpectrum.get_data()featuresexclude="bads'BaseTFRdittohoechenberger commentedon Apr 26, 2024
My feeling is that we should add
excludeparameters to all theseget_data()methods if they don't have one already, retaining the current behavior; and do a deprecation cycle for transitioning to eitherexclude=()orexclude="bads"for all of themRenanFlorencio commentedon Nov 6, 2025
Hello,
Me and two friends (@luisademelo14 @Fooxfeir) are taking a course and we need to choose a tool and bug to fix. We chose MNE-Python, but since we are new to the community, we decided to pick a bug marked as easy and this one seems good.
If there's a problem with that, please let us know!
larsoner commentedon Nov 7, 2025
#12197 is also related
FYI not all of this issue is easy... the first part is probably uncontroversial: namely adding
excludeparams to functions that don't have them, retaining old behavior, and documenting things properly (#12197). I think documenting current behavior and making it changeable by addingexclude=...in places we don't have it is already a good start here, so @RenanFlorencio @luisademelo14 @Fooxfeir I think you can work on this part! 👍 So feel free to start on thatThe deprecation cycle to transition them all to have the same behavior will not be trivial. I suspect a lot of tests will need to be adjusted, and it will be hard to do without a reasonably deep working knowledge of the code.
I am also not 100% sure we should do the deprecation to make everything have either
exclude=()orexclude="badseverywhere. There are probably functions and classes where we should exclude bads -- like in multivariate connectivity fitting for example we've talked about it and there are probably other examples -- and other functions/classes (like constructing epochs maybe) it is convenient to include them by default. We'd have to look at all use cases (decoding, inverses, stats, time-freq/spectra, connectivity, etc.) to see what makes sense here. Moreover, if we change the behavior of something likeEpochsthe risk of breaking people's code is quite high even with a deprecation cycle... so we should think really hard about whether the potential disruption is worth the uniformity, and how much it buys us.RenanFlorencio commentedon Nov 11, 2025
Okay @larsoner , we worked on the first part and we added the exclude parameter to all three functions (
epochs,evokedandbase, sincespectrumandtfralready have it), this way we have three different behaviors for each. We would like you to confirm this is what's expected to write the docstring. We also want to ask you a question that is at the end of this comment.Maintained Previous Behavior:
Results in:
Behavior when exclude=()
Now, both functions will not exclude bad channels:
Gives us:
Behavior when exclude="bads"
Now, both functions will exclude bad channels:
Resulting in:
Question
Lastly, we would like to ask you about the following lines of code in
epochs.pyat the_get_datafunction:Here you can see that we changed the line inside the
elseblock to add theexcludeparameter. However, theifblock is still "hard coded" toexclude=(). Should it also use the newexcludeparameter? If so, we would have to figure out a way to keep the old behavior, since theelseblock would need a defaultexclude=bads, which we are using, and theifblock would needexclude=().Conclusion
There are two functions, in
rawandevoked, use defaultexclude=(). The other three, inepochs,spectrumandtfruse default `exclude='bads'. If we were to do a deprecation cycle, we would have to think more about which one makes more sense. Moreover, do you have any suggestions of issues we could work on?larsoner commentedon Nov 25, 2025
For this, rather than using
printhow about you write some tiny test onmainthat should pass thatasserts something about shapes of returned data?Then you add the
excludeparam defaults, and your test ensures that you haven't changed behavior.Then you append to that same test some calls with different
excludevalues andassertsomething new/different about the obtained shapes.This is a variant of test-driven development (strictly speaking there you'd write the whole test first even though it would fail, then modify code, but in practice it's nice to see stuff green / passing to start)
I would try not to mess with this one. Internal functions get split to handle specific use cases... the
epochscode gets used all over the place so at the end of the day having CIs/tests green should tell us if things are okay or not.Let's not do that for now... I think it would be more end-user pain that it's worth.
Anything with the
EASYlabel should in principle be somewhat easy, at least that's the goal 😄https://github.com/mne-tools/mne-python/issues?q=state%3Aopen%20label%3AEASY
Update evoked.py