Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
CarinaFo
left a comment
There was a problem hiding this comment.
Looks good, some small nitpicks.
We can use _pl from mne.utils to use the singular/plural of channel depending on length of bad channels.
Thank you for your contribution.
| that to aid in interpolation rather than completely discarding the | ||
| data from the two channels. | ||
|
|
||
| Channels listed in ``inst.info["bads"]`` are ignored for exclusion and |
There was a problem hiding this comment.
| Channels listed in ``inst.info["bads"]`` are ignored for exclusion and | |
| Channels listed in ``inst.info["bads"]`` are not excluded and |
| from ..io import BaseRaw, RawArray | ||
| from ..transforms import _cart_to_sph, _sph_to_cart | ||
| from ..utils import _ensure_int, _validate_type | ||
| from ..utils import _ensure_int, _validate_type, warn |
There was a problem hiding this comment.
| from ..utils import _ensure_int, _validate_type, warn | |
| from ..utils import _ensure_int, _pl, _validate_type, warn |
| bads_orig = inst.info["bads"] | ||
| if bads_orig: | ||
| warn( | ||
| f"The channels {', '.join(bads_orig)} are marked as bad but will not " |
There was a problem hiding this comment.
| f"The channels {', '.join(bads_orig)} are marked as bad but will not " | |
| f"The channel{_pl(bads_orig)} marked as bad will not " |
e0f5358 to
e7f7d7d
Compare
CarinaFo
left a comment
There was a problem hiding this comment.
looks good, thanks again for your contribution.
| @@ -0,0 +1 @@ | |||
| Warn when :func:`mne.preprocessing.interpolate_bridged_electrodes` ignores bad channels during interpolation, by `Deepesh Sonar`_. | |||
There was a problem hiding this comment.
| Warn when :func:`mne.preprocessing.interpolate_bridged_electrodes` ignores bad channels during interpolation, by `Deepesh Sonar`_. | |
| Warn when :func:`~mne.preprocessing.interpolate_bridged_electrodes` ignores bad channels during interpolation, by `Deepesh Sonar`_. |
tilde will suppress the mne.preprocessing. part in the rendered HTML
| if bads_orig: | ||
| warn( | ||
| f"The channel{_pl(bads_orig)} marked as bad will not " | ||
| "be excluded from bridged-electrode interpolation and may influence " | ||
| "the result." | ||
| ) |
There was a problem hiding this comment.
If your assessment of the problem is correct, then I think this is probably not the right solution. It seems very unlikely that users will want information from bad channels to be used this way. Wouldn't it be better to actually prevent bad channels from contributing to the interpolation?
There was a problem hiding this comment.
Good point. I looked into preventing the existing bad channels from contributing rather than just warning about it.
interpolate_bads() uses info["bads"] to determine both the interpolation targets and, by exclusion, the source channels. So simply keeping the pre-existing bads in info["bads"] would also interpolate those channels, which we don't want.
I think a minimal fix would be to keep the bridged channels as the interpolation targets, and pass only the pre-existing non-bridged bad channels via interpolate_bads(exclude=...). That would make those channels neither sources nor targets, while leaving their data untouched and preserving the existing bridged-channel behavior.
I also checked the overlap case where a channel is both bridged and already marked bad. There is a separate pre-existing behavior where its data contributes to the virtual-channel average. The filtered exclude= change wouldn't alter that behavior, so I'd leave that out of scope here unless you think it should be handled as part of this fix.
Does that approach sound right?
There was a problem hiding this comment.
My initial reasoning for not excluding bads here was that a user might do bridging detection as part of their first quality check of channels. If we then exclude anything in bads, we might potentially be excluding exactly the channels this function wants to rescue, undermining the aim of this function.
I think the open question is what bads actually represent in a typical workflow: is it (a) a channel list where bad-marking is reserved for non-bridging issues (noise, disconnection, flat signal) (then excluding bads is of course a good idea) or (b) a mixed list of bad channels where bridged channels are part of the first QC, excluding them would then be wrong. I honestly don't know which workflow is more common in practice.
Or maybe I am missing something crucial here?
There was a problem hiding this comment.
I think that distinction is exactly why we shouldn't exclude all of bads_orig. What I was proposing above is to exclude only the pre-existing bad channels that are not part of the bridged set.
So if EEG 001 is both in info["bads"] and detected as bridged, it would remain an interpolation target and still be repaired as it is today. If EEG 003 is in info["bads"] but is not bridged, it would be passed via exclude= so it cannot contribute to the interpolation and its data remains untouched.
There is still an ambiguity if a bridged channel was independently marked bad for some other reason (e.g. noise), since info["bads"] doesn't encode why a channel was marked bad. I think resolving that would require a separate semantic decision. The filtered exclusion would at least leave the existing behavior for bads ∩ bridged unchanged while preventing known non-bridged bads from contributing.
Does that address the workflow you're concerned about?
There was a problem hiding this comment.
Yes, that actually makes sense, thanks for clarifying that. I support the suggested change.
e7f7d7d to
81161cb
Compare
interpolate_bridged_electrodes cleared inst.info['bads'] while building the interpolation, so channels already marked bad could contribute to the bridged interpolation without the caller knowing (mne-toolsgh-14263). Keep bridged channels as the interpolation targets, but pass pre-existing bad channels that are not part of the bridged set via interpolate_bads(exclude=...), so they are neither interpolation sources nor targets and their data is left untouched. Channels that are both bridged and marked bad remain interpolation targets, as before.
81161cb to
ef9cadf
Compare
| that to aid in interpolation rather than completely discarding the | ||
| data from the two channels. | ||
|
|
||
| Channels in ``inst.info["bads"]`` that are not part of the bridged |
There was a problem hiding this comment.
| Channels in ``inst.info["bads"]`` that are not part of the bridged | |
| .. versionchanged:: 1.14 | |
| Pre-existing bad channels that are not part of the bridged set are | |
| now excluded from the interpolation. |
CarinaFo
left a comment
There was a problem hiding this comment.
Thanks again for implementing the requested fix, just a few minor comments on docs and the test, which can be a bit more verbose.
|
|
||
| def test_interpolate_bridged_electrodes(): | ||
| """Test interpolate_bridged_electrodes function.""" | ||
| raw, epochs, evoked = _load_data() |
There was a problem hiding this comment.
I think we can get rid of the loop and just test on raw, the behaviour of interpolate_bridged_electrodes does not depend on the input type.
| inst2.info["bads"] = ["EEG 001", "EEG 002", "EEG 003"] | ||
| inst2.interpolate_bads() | ||
| data_interp_reg = inst2.get_data(picks=["EEG 001", "EEG 002"]) | ||
| # a pre-existing bad that is not bridged must not contribute: poisoning |
There was a problem hiding this comment.
| # a pre-existing bad that is not bridged must not contribute: poisoning | |
| # Verify non-bridged bad doesn't influence the interpolation result |
| assert 1e-6 < np.mean(np.abs(data_interp - data_interp_reg)) < 5.4e-5 | ||
| assert 1e-6 < np.mean(np.abs(data_interp - data_interp_reg)) < 6.5e-5 | ||
|
|
||
| # a channel that is both pre-existing bad and bridged must still be |
There was a problem hiding this comment.
| # a channel that is both pre-existing bad and bridged must still be | |
| # Bridged channel that is also marked bad should be repaired |
Summary
interpolate_bridged_electrodestemporarily clearsinst.info["bads"]while building the interpolation. This can allow channels already marked bad to influence the result without notifying the caller.This patch keeps the existing interpolation behavior and public API unchanged, but emits a warning naming the affected channels and documents the behavior.
Changes
inst.info["bads"]is non-empty.Testing
.venv/bin/python -m pytest -p no:pytest-qt mne/preprocessing/tests/test_interpolate.py::test_interpolate_bridged_electrodes --verbose— 1 passed.venv/bin/python -m pytest -p no:pytest-qt mne/preprocessing/tests/test_interpolate.py --verbose— 11 passed.venv/bin/python -m pytest -p no:pytest-qt mne/tests/test_docstring_parameters.py --verbose— 17 passed, 2 skippedgit diff --checkpassedCloses #14263.