Fix bug in _project_onto_surface

[christian-oreilly]

FIx a bug when calling _project_onto_surface with method != "accurate" and return_nn == True.

[christian-oreilly]

@larsoner These CI fails do not seem to be related to this PR (i.e., moved one line of code two lines above (from inside to outside an if-else statement). How do you normally get these passed?

[larsoner]

The errors seem related to this PR. This one for example:

mne/tests/test_source_estimate.py:1568: in test_stc_near_sensors
    stc = stc_near_sensors(evoked, **kwargs)
<decorator-gen-335>:10: in stc_near_sensors
???
mne/source_estimate.py:3364: in stc_near_sensors
    method='nearest')[2]
mne/surface.py:318: in _project_onto_surface
    surf_geom = _get_tri_supp_geom(surf)
mne/surface.py:1403: in _get_tri_supp_geom
    r1 = surf['rr'][surf['tris'][:, 0], :]
E   KeyError: 'tris'

looks like it's because you deindented / moved out of the conditional the _get_tri_supp_geom. You can check locally -- pytest mne/tests/test_source_estimate.py -k near_sensors should fail locally but pass if you revert your change (by putting it back in the conditional). This function is private so corner cases like this have not been tested, probably because where we use this private function interally we know that we don't need the tris and rr are enough for example.

Why do you need this functionality changed? You shouldn't be using this private function in your own code, it should only be used internally by MNE-Python. Do you hit it somehow using a public API?

[christian-oreilly]

Thanks for the feedback, @larsoner. Sorry about that; I should have better checked the log of the CI to debug it myself. I was working on faulty assumptions.

Nop. This error was not reached through the use of the public API, and don't worry, I am well aware that there is no guarantee of stability on private methods. Regardless of the fact that it is a corner case, it seemed to me that if there was an erroneous piece of logic in the code it was best to fix it for all than just fix it on my side... just for the sakes of code base robustness.

These changes fixed the CI issues, except for this error which does not seem to be related to my edit (?):

mne\fixes.py:462: in __repr__
    from sklearn.base import _pprint
E   ImportError: cannot import name '_pprint' from 'sklearn.base' (C:\hostedtoolcache\windows\Python\3.10.5\x64\lib\site-packages\sklearn\base.py)

[agramfort]

do we know why it was not detected by current tests?

[christian-oreilly]

Not sure, but it would be simple to test.

FYI, in my case, I was just needing to display an overlay of a net on a head for a small GUI app that I coded to digitize electrode positions with the Polhemus Fastrak system (https://github.com/christian-oreilly/fastrak_digitizer/blob/main/fastrakdigitizer/main.py#L532). Somehow, in some instances, the accurate method was taking forever whereas the nearest neighbor method was very fast and I did not need accuracy for this operation since it is just a visualization to guide the user so I don't care for it to be precise to the mm.

[drammock]

These changes fixed the CI issues, except for this error which does not seem to be related to my edit (?):

that one should be fixed now by #10933, so if you merge in (or rebase against) main it should pass now.

[agramfort]

@drammock merge if happy

[drammock]

would still be nice to add a test that failed on main and passes here. @christian-oreilly you said it should be simple to test, can you add one?

[christian-oreilly]

Something like that would test that. Check if that code seems kosher to your @drammock , and if so, I can wrap it in a test function and commit it.

from pathlib import Path
import mne
from mne.datasets import sample
from mne.transforms import _get_trans, transform_surface_to, apply_trans
from mne.bem import read_bem_surfaces
from mne.surface import _project_onto_surface
import pandas as pd
import numpy as np

data_path = Path(sample.data_path())
surf_path = data_path / "subjects" / "sample" / "bem" / "sample-head.fif"

trans_fname = data_path / 'MEG' / 'sample' / 'sample_audvis_raw-trans.fif'
trans = mne.read_trans(trans_fname)
head_surf = transform_surface_to(surf=read_bem_surfaces(surf_path)[0],
                                 dest="mri",
                                 trans=_get_trans(trans, 'head', 'mri'),
                                 copy=True)
                   
montage = mne.channels.make_standard_montage('GSN-HydroCel-129')
info = mne.create_info(montage.ch_names, 100, ch_types='eeg')
raw = mne.io.RawArray(np.zeros((len(montage.ch_names), 100)), info)
raw.set_montage(montage)

mri_fiducials = mne.coreg.get_mni_fiducials("sample", data_path / "subjects")
mri_fid_loc = pd.DataFrame(np.array([fid["r"] for fid in mri_fiducials]),
                           index=[fid["ident"]._name.split("_")[-1] for fid in mri_fiducials],
                           columns=["x", "y", "z"])

eeg_picks = mne.pick_types(info, meg=False, eeg=True, ref_meg=False)
eeg_loc = np.array([raw.info['chs'][k]['loc'][:3] for k in eeg_picks])

cap_fid_loc = pd.DataFrame(np.array([fid["r"] for fid in raw.info["dig"][:3]]),
                           index=[fid["ident"] for fid in raw.info["dig"][:3]],
                           columns=["x", "y", "z"])

trans = mne.coreg.fit_matched_points(cap_fid_loc.values, mri_fid_loc.values, 
                                     out='trans', scale=True)

eeg_loc = apply_trans(trans, eeg_loc)
eeg_loc = _project_onto_surface(eeg_loc, head_surf, project_rrs=True,
                                return_nn=True, method="nearest neighbor")[2]    

[drammock]

looks reasonable. Couple of quick comments:

1. no need to wrap sample.data_path() in Path(); all our data_path() functions return pathlib objects now.
2. for the two pandas dataframes you create (mri_fid_loc and cap_fid_loc), only their .values are used; why not create as numpy arrays directly, and avoid the overhead of importing pandas?
3. I don't see any assert statements, so I'm assuming that on main it is the last line (the call to _project_onto_surface) that fails? What is the error? EDIT: never mind, I tried locally and see that you get UnboundLocalError: local variable 'surf_geom' referenced before assignment

[drammock]

side comment: here's why we never caught this before: _project_onto_surface is only used in 3 places in our codebase, and only one of those specifies return_nn=True, but that time it uses the default method='accurate' (not 'nearest').

[christian-oreilly]

1 - Great
2 - Sure.
3 - Yes, as you've seen, this was the error.

Side comment: yeah... that is part of the exchange we had with @larsoner where he was saying it is a corner case, currently impossible to reach from the public interface. Nevertheless, if it raises an exception, I guess it is worth fixing, 1) for people like me who hack their way around (after all, this is a toolbox used mainly for research I expect, so people hacking its inner functions to make them do things not currently supported by the public interface is to be expected) and 2) it makes it more robust to future development which may call this function internally for new use cases/functionalities...

I'll commit the test.

[drammock]

@christian-oreilly just FYI, _get_fid_coords is a useful private func for testing, to replace the list comprehensions you were using

[christian-oreilly]

Thanks for the tip. Nice thinking for generalizing the test (i.e., @pytest.mark.parametrize('ret_nn', (False, True)),
@pytest.mark.parametrize('method', ('accurate', 'nearest'))).

[drammock]

only remaining CI failures will be addressed by #10944, so this is good to go! Thanks @christian-oreilly