DOC: clarify index mismatch in Epochs.drop when epochs are auto-dropped#13688
DOC: clarify index mismatch in Epochs.drop when epochs are auto-dropped#13688drammock merged 14 commits intomne-tools:mainfrom
Conversation
Famous077
requested review from
agramfort,
drammock,
larsoner and
mscheltienne
as code owners
mne/epochs.py
Outdated
| If some epochs were automatically dropped (e.g., due to | ||
| insufficient data or rejection), the indices shown in | ||
| :meth:`mne.Epochs.plot` may not match the indices expected | ||
| by this method. The plot shows indices from | ||
| :attr:`mne.Epochs.selection` (original event indices), | ||
| while this method uses 0-based indices into the current | ||
| set of remaining epochs. To convert plot indices to drop | ||
| indices, use:: | ||
|
|
||
| # epochs.selection contains the original indices | ||
| # of the remaining epochs | ||
| plot_idx = 1 # index shown in epochs.plot() | ||
| drop_idx = np.where(epochs.selection == plot_idx)[0] | ||
| epochs.drop(drop_idx) | ||
|
|
There was a problem hiding this comment.
This is pretty good. I think:
- it should be in a separate
Notessection of the docstring - it should emphasize less "automatic dropping" and "what is shown in epochs.plot" and instead focus on the fact that this method expects zero-based indices into the currently remaining epochs (not the epochs' original indices, as found in
epochs.selection).
|
Hi @drammock , Thanks for the feedback. I've moved the content into a separate |
|
Not sure why the docs aren't building here, but if I run locally, it complains about @drammock, is it worth making |
IMO: not worth it just for xref |
tsbinns
left a comment
tsbinns
left a comment
There was a problem hiding this comment.
@Famous077 Some suggestions to get the docs rendering properly. Apart from that, I think this is looking good!
doc/changes/dev/13688.other.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Add a note to :meth:`~mne.Epochs.drop` describing a potential mismatch between indices expected by :meth:`~mne.Epochs.drop` and those found in :attr:`~mne.Epochs.selection` when epochs have been automatically dropped, by :newcontrib:`Famous Raj Bhat`. No newline at end of file | |||
There was a problem hiding this comment.
| Add a note to :meth:`~mne.Epochs.drop` describing a potential mismatch between indices expected by :meth:`~mne.Epochs.drop` and those found in :attr:`~mne.Epochs.selection` when epochs have been automatically dropped, by :newcontrib:`Famous Raj Bhat`. | |
| Add a note to :meth:`mne.Epochs.drop` describing a potential mismatch between the indices expected by :meth:`~mne.Epochs.drop` and those found in the ``selection`` attribute when epochs have been automatically dropped, by :newcontrib:`Famous Raj Bhat`. |
mne/epochs.py
Outdated
| ----- | ||
| This method expects zero-based indices into the currently remaining | ||
| epochs, not the original event indices (as found in | ||
| :attr:`mne.Epochs.selection`). If some epochs were automatically |
There was a problem hiding this comment.
| :attr:`mne.Epochs.selection`). If some epochs were automatically | |
| the ``selection`` attribute). If some epochs were automatically |
There was a problem hiding this comment.
@tsbinns Update done. Waiting for you review .
tsbinns
left a comment
tsbinns
left a comment
There was a problem hiding this comment.
Thanks @Famous077! I'm happy with this, and it seems to me that @drammock's earlier points have been addressed, so I'll approve, though they will also need to do so before this gets merged.
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
LGTM. I've suggested some fine-tuning of the wording, which I'll commit, and then mark for auto-merge.
|
Hi @tsbinns , The CI failures are unrelated to this PR's changes. They're caused by a network connectivity issue in the Azure Pipelines runner — codeberg.org (a nibabel submodule host) is unreachable during dependency installation. Could you please re-run the failed checks or override them and let me know everything is working fine or not. |
3 participants
Fixed issue #11373
-->
What does this implement/fix?
--> The indices shown in epochs.plot() reflect epochs.selection
(original event indices), while epochs.drop() uses 0-based
indices into the remaining epochs. This PR clarifies this
behavior in the docstring and adds an example showing how to
correctly convert plot indices using epochs.selection.
Additional information
--> only documentation, No additional changes to code logic.