[BUG] Fix bad channels error, better default #12382
Conversation
alexrockhill
requested review from
larsoner,
drammock,
agramfort and
dengemann
as code owners
mne/source_estimate.py
Outdated
| @@ -3781,7 +3781,7 @@ def stc_near_sensors( | |||
| subjects_dir=None, | |||
| src=None, | |||
| picks=None, | |||
| surface="pial", | |||
| surface=None, | |||
There was a problem hiding this comment.
IIRC there was logic to the original choice -- source spaces are almost always created with white as the surface (for inverse modeling) but non-inverse-based projections like this should almost always use pial. So there was some logic to it. Not sure we should make this change
There was a problem hiding this comment.
Why would you model with white and then project to pial?
There was a problem hiding this comment.
So far when I've used this function I haven't used the source space for anything other than getting a nice decimation, so calling setup_source_space(subject, subjects_dir=subjects_dir) and everything works fine (things are projected to pial). With this change that code will be broken because now the projection will happen to white instead, since this is the default for setup_source_space. I agree that it's more consistent for people to do setup_source_space(subject, surface="white", subjects_dir=subjects_dir) then this function will behave as expected but I don't think the current implementation will make it clear that's what will be done. Hence why I think we maybe shouldn't bother changing it. If we do change it, I think we need to properly deprecate because people like me would otherwise silently get a different (wrong!) result, namely projection to the white surface whereas before it projected (correctly) to pial when I ask for a decimated version by passing src.
There was a problem hiding this comment.
That seems very confusing. Maybe you shouldn't be allowed to pass a surface at all but should just use the surface in the source space?
There was a problem hiding this comment.
That would have the same backward compat problem
There was a problem hiding this comment.
For deprecations the policy is to preserve the old/current behavior for the upcoming release (1.7), but warn that the default will change in the next only if the behavior will actually change in the next release (1.8) given the current function inputs being passed.
So for example you would warn if someone did setup_source_space(subject) (which defaults to surface='white') and stc_near_sensors(evoked, src=src) (because in 1.7 the default is pial but in 1.8 the behavior under None would change to white); but you would not warn if they did setup_source_space(subject, surface='pial') then stc_near_sensors(evoked, src=src) because the behavior would be the same in 1.7 and 1.8. I think this can be accomplished with pseudocode:
def stc_near_sensors(..., surf=None):
...
if surf is None:
if src is not None:
src_surf = ...
if src_surf != "pial"
warn(..., DeprecationWarning)
surf = "pial"
then after 1.7 comes out we simplify to
if surf is None:
if src is None:
surf = "pial"
else:
surf = ... # src_surf
There was a problem hiding this comment.
I'm not sure that the surface is stored in the src instance. I looked and couldn't find it and it looks from setup_source_space that the surface is not stored as far as I can tell... so then you can't fill in the src_surf = .... line. Any ideas on how to handle that?
There was a problem hiding this comment.
You could try/except to load the lh.pial surface. If it raises an exception (like file not found) or not np.allclose(src[0]['rr'], pial_rr) you could conclude it's not pial. Hopefully not too painful (5ish lines?) but would do the trick I think.
There was a problem hiding this comment.
Yikes okay well at least it will only in the codebase for a version
There was a problem hiding this comment.
Ok, I didn't end up doing it exactly how you suggested but I did it so that if you had no argument for surface and passed a src then you would get a warning that the behavior will change but in this version the behavior doesn't change which is what you asked for.
mne/tests/test_source_estimate.py
Outdated
| import pdb | ||
|
|
||
| pdb.set_trace() |
There was a problem hiding this comment.
Thanks for the review, sorry for the bad mistake!
doc/changes/devel/12382.bugfix.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Fix bad channels not handled properly in :func:`mne.stc_near_sensors` as well as default ``surface`` handling for :class:`mne.VolSourceEstimate` inputs as ``src``, by `Alex Rockhill`_. | |||
There was a problem hiding this comment.
Not just a bugfix because it's involves a deprecation cycle. So the more important naming and change I think would be 12382.apichange.rst describing how behavior is being changed in stc_near_sensors. Then you can have a 12382.bugfix.rst to describe the bug fix separately if you want (but less critical than calling out the API change via deprecation in the right place in the changelog)
for more information, see https://pre-commit.ci
|
Ok I good by me |
|
Thanks @alexrockhill ! FYI to avoid the pre-commit bot having to do stuff you can do locall |
|
@alexrockhill I suspect this broke circle can you check? |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Fixes #12351