Raising meaningful warnings/errors for interpolate_bads, when supplie…

[1himan]

Raising meaningful warnings/errors for interpolate_bads, when supplied loc field with missing or invalid data.

What does this implement/fix?

Fix - raise error in interpolate_bads if no sensor positions are provided

Additional information

TODO:
Add checks in add_channels() as well.
Refactor code according to mne's philosophy.

[1himan]

Should we update the user about what's going on, on a deeper level or is it going to be too verbose.

Because if we look, that why the bug exists, its primarily because of how create_info creates the info dict structure - its all NaN for the loc field. Of course we're not expecting the user to provide the exact positions of the sensor but we should also give the user a heads up about this.

Also in the add_channels, something like:
Warning: Channel 'MEG2112' was added without valid sensor position (loc) information. Interpolation or spatial filtering will fail silently or raise an error unless position data is manually supplied.

And something similar for the interpolate_bads.

If that's the case then we should create a separate utility method for this, something like:

def _is_invalid_loc(loc):
    return np.allclose(loc, 0.0, atol=1e-16) or np.isnan(loc).any()

And use that, wherever its required to warn the user about this.

[scott-huberty]

@1himan I haven't followed this ticket super closely but if I understand correctly it seems like you are talking about 2 or 3 different ideas here:

1: "Should we update the user about what's going on on a deeper level [during the interpolate_bads_function]"

Generally We want to:

1. Let the user know what is wrong, e.g. "We don't have the spatial coordinates for bad sensor(s) ['X1', 'X2'] so we can't interpolate them using the signal of neighboring channels".
2. Let the user know how to fix or work around it.. something like "Please use the set_montage method to set the sensor locations, or pass on_no_position='warn', which will set bad channel(s) ['X1', 'X2'] to np.nan" .

We don't want to bog the user down with details about the loc array because I don't think that we expect the typical user to even know about it, let alone manually change it. If you want to communicate details that can help with debugging to super-users or other devs, you can use logger.debug

2: Should we add a similar warning to add_channels (if the user adds a channel without providing spatial sensor coordinates)

I'm -1 on this. Not all MNE functionality requires sensor locations, so I don' think we need to eagerly warn about missing sensor info during add_channels. I think it's okay to stay focused on the interpolation use-case, and if down the road we decide that we want raise/warn in other places, then we can put this warning message in a re-usable helper function!

3: the bug exists primarily because of how create_info creates the info dict structure

I'm not sure I agree, but anyways I don't think you need to worry about the internals of create_info!

Once you address/integrate the open maintainer feedback, or once you have other questions for, feel free to ping us!

[1himan]

Ready to run rest of the tests?

[drammock]

Thanks for the contribution!

there should be a test (or modification of an existing test) to ensure that this works as intended. For example, a pytest.raises() context around a call when the value is "raise", a pytest.warns() context around a call when the value is "warn", and no context (or a null context) when called with the value "ignore". Should also assert that the invalid chs end up as all NaN (though since that's current behavior I'm guessing that's already tested, but please confirm)

[1himan]

I've made the suggested changes @drammock. and yes, the assertion for bad channel interpolated to all nan values is already implemented.