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?

Fixes #13363

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.

[drammock]

Looking more closely this time... I'm not sure what the point of this assert statement is. We don't expect interpolate_bads to ever change the loc of a channel, so what is the point of asserting this here?

[1himan]

thanks for correcting, what I really wanted to do here is - to assert that the interpolated channel is all nan, which is the current behaviour and I got too fixated on the sensor position. So instead of that, we would want something like this:

assert np.isnan(bad_chs).all, "Interpolated channel should be all nan"

[drammock]

two thoughts about this:

1. if the user passed "warn" they're going to see this message as a warning, which means that the interpolation will proceed, even though the text implies that it will not. That's confusing.
2. in the case where method="nan" it's actually quite plausible to want to apply this to channels with invalid locations. So I wonder if we want to issue this warning at all if method="nan". My hunch is that we probably still do want to warn, but would like other opinions (@larsoner ?)

[1himan]

The original issue which this PR is meant to fix, was more of a UX improvement request, that we should at least raise a warning when interpolating a channel with bad sensor positions instead of silently interpolating the channel to np.nan.

1. "ignore" - interpolate anyways
2. "warn" - interpolate anyways as well, but leaves a warning message in terminal
3. "raise" - no interpolation, stops the code execution and gives a runtime warning

To make it less confusing, I think, the default value should be on_bad_position="raise" instead of "warn".
Because, for most of the users who happened to have the data with invalid sensor positions, the workflow would look like:

1. Trying to interpolate bad channels via .interpolate_bads()
2. Bumps into this runtime error - because default is on_bad_position="raise"
3. Now they can either choose to "ignore" it, or some users(who know what they're doing) might wanna use "warn" just as a reminder/note. Apart from that I don't see "warn" to be of any use, or any case where the user want to deliberately set it?

[1himan]

Also in the end it just depends on how we want things to be, "raise" could be a default for "better UX" since its good to tell users, "hey, there's something wrong with your code, so we're stopping the code execution immediately, see below for possible fixes", and then show a meaningful error message in the terminal and tell them to either use "warn" or "ignore", to continue.

But if we don't wanna:

1. throw a WHOLE runtime error to the user and,
2. stop the code execution immediately

for "better UX", we might wanna use "warn" as default.

Its just a choice of what we consider "better UX" to be?