ENH: Add on_drop_all parameter to Epochs.drop#13556
ENH: Add on_drop_all parameter to Epochs.drop#13556shruti423 wants to merge 24 commits intomne-tools:mainfrom
Conversation
shruti423
requested review from
agramfort,
dengemann,
drammock and
mscheltienne
as code owners
for more information, see https://pre-commit.ci
|
you're going to need to add tests. |
…ython into warn-epochs-drop
for more information, see https://pre-commit.ci
|
I have added the test case in mne/tests/test_epochs.py as requested. It covers warn, raise, ignore, and invalid input scenarios. |
mne/tests/test_epochs.py
Outdated
| def test_drop_all_epochs(): | ||
| """Test on_drop_all parameter in Epochs.drop.""" | ||
| # Create tiny dummy data (3 epochs) | ||
| data = np.random.randn(3, 1, 10) |
There was a problem hiding this comment.
use a random state to avoid touching the global seed.
| data = np.random.randn(3, 1, 10) | |
| data = np.random.RandomState(0).randn(3, 1, 10) |
There was a problem hiding this comment.
also you can use only 1 epoch and not 3. It's a good habit to take to think about keep the test time as low as possible
|
I switched to RandomState(0) and cut it down to 1 epoch. Thanks for the tip on keeping the tests fast—that's a good habit to build. Well I had a doubt regarding the number of epochs, I know most standard experiments usually have a few hundred epochs, but do MNE users often work with massive datasets (like thousands of epochs for sleep scoring)? I'm just trying to get a better idea about how aggressive I should be with optimization in future PRs |
…ython into warn-epochs-drop
…ython into warn-epochs-drop
for more information, see https://pre-commit.ci
|
@larsoner You were right! The default on_drop_all='warn' was indeed breaking existing plotting tests. I've switched the default to 'ignore' and updated the new tests to cover all cases explicitly. All relevant checks are passing now. Thanks! |
|
|
||
| @verbose | ||
| def drop(self, indices, reason="USER", verbose=None): | ||
| def drop(self, indices, reason="USER", verbose=None, on_drop_all="ignore"): |
There was a problem hiding this comment.
I think the default should be "warn".
| def drop(self, indices, reason="USER", verbose=None, on_drop_all="ignore"): | |
| def drop(self, indices, reason="USER", verbose=None, on_drop_all="warn"): |
| If 'ignore', no error is raised and the epochs object is empty. | ||
| If 'warn', a RuntimeWarning is emitted. | ||
| If 'raise', a ValueError is raised. | ||
| Default: 'ignore'. |
There was a problem hiding this comment.
| Default: 'ignore'. | |
| Default: 'warn'. |
sappelhoff
left a comment
sappelhoff
left a comment
There was a problem hiding this comment.
LGTM, provided tests come back green.
But I would like the default to be changed to "warn".
4 participants
Implements the on_drop_all parameter to warn or raise an error when all epochs are dropped, as discussed in #13473.
Epochs.dropshould warn if all epochs would be dropped #13473