[MRG] Suggest alternatives when value not found in list

[larsoner]

PR Description

When lst.index(val) fails (e.g., subjects.index(subject)), try to help people out with suggestions. I was on this PR when I got the traceback in #1065:

ValueError: 'sub-001' is not in list. Did you mean one of ['CQ2', 'BT4', 'CQ1', 'CQ3', 'CP9', 'CA7', 'CB6', 'BV2', 'CP8', 'CP1', 'CP2', 'CO9', 'CP5', 'CL1', 'BL7', 'CO3', 'CE2', 'CV6', 'CJ2', 'CY8']?

I figured subject lists are generally short enough to allow_all to be given (if there are no close matches), but wasn't sure about the others. But they can easily be added elsewhere or tweaked in these cases by follow-up PRs by heavier/more experienced users and devs of MNE-BIDS.

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

• All comments are resolved
• This is not your own PR
• All CIs are happy
• PR title starts with [MRG]
• whats_new.rst is updated
• New contributors have been added to CITATION.cff
• PR description includes phrase "closes <#issue-number>"

[larsoner]

... on main the error message is just:

ValueError: 'sub-001' is not in list

which I think is much less helpful.

[codecov]

Codecov Report

Merging #1066 (4f459f6) into main (7eb37ef) will increase coverage by 1.10%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##             main    #1066      +/-   ##
==========================================
+ Coverage   93.95%   95.06%   +1.10%     
==========================================
  Files          25       25              
  Lines        3822     3850      +28     
==========================================
+ Hits         3591     3660      +69     
+ Misses        231      190      -41     

Impacted Files	Coverage Δ
mne_bids/read.py	94.76% <53.84%> (-1.34%)	⬇️
mne_bids/path.py	97.58% <0.00%> (-0.10%)	⬇️
mne_bids/config.py	97.64% <0.00%> (+0.02%)	⬆️
mne_bids/utils.py	95.74% <0.00%> (+0.53%)	⬆️
mne_bids/write.py	97.17% <0.00%> (+1.93%)	⬆️
mne_bids/dig.py	98.27% <0.00%> (+9.65%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hoechenberger]

I love this idea. Just one comment

[larsoner]

I suspect the failures in https://github.com/mne-tools/mne-bids/runs/8286821887?check_suite_focus=true are due to:

• BUG: Fix bugs with digitization mne-python#11146

where EEG positions in chs[idx]['loc'][:3] are now put in info['dig'] to keep things consistent. I'm not quite sure the right way to deal with this in MNE-BIDS, though. Should we just ignore the warning about not having fiducials in the tests?

[agramfort]

doing

mne.io.read_raw_cnt(os.path.expanduser("~/mne_data/MNE-testing-data/CNT/scan41_short.cnt")).info

replicates the problem. You see that there is no fiducials read from the files.

I see two options:

1. we make it just a warning and not a ValueError in write_raw_bids
   and we filter it in the test or
2. we consider than having dig in head after mne.io.read_raw_cnt while fiducials are not present makes little sense as head is defined from fiducials.

I am leaning towards the 2nd solution.

thought?

[hoechenberger]

Pinging @sappelhoff

[sappelhoff]

in principle I am also leaning towards @agramfort's 2nd solution ... except that one may get coordinates that are already in "head" (which is the same as "captrak", and just in general a sensible /obvious way to define coordinates) without fiducials --> these are still valid, even if no fiducials are present, no?

[agramfort]

can you come up with good fiducials automatically for captrack?

…

Message ID: ***@***.***>

[sappelhoff]

can you come up with good fiducials automatically for captrack?

it's the same as head, LPA is at (-1, 0, 0), RPA is at (1, 0, 0), NAS is at (0, 1, 0) ... not sure if it's possible to estimate the positions based on that. probably not

[sappelhoff]

@agramfort @larsoner @hoechenberger I think we should agree on a fix for this problem, because our regular CI fails because of it for many days now :-)

Taking @agramfort's comment:

• Is the present problem only an issue for CTF data?
• Taking Alex's point 2 --> how can CTF have dig in head when there are no fiducials present?

how do you guys think we should fix it?

[larsoner]

Taking Alex's point 2 --> how can CTF have dig in head when there are no fiducials present?

To be clear we're talking about CNT not CTF. CTF usually has fiducials IIRC because the HPI coils are placed there.

FWIW I'm happy with putting these in UNKNOWN coord frame instead as kind of a bug fix. Previously the behavior was just to put locations in info['chs'][ii]['loc'][:3] and not in dig, and info['chs'] for EEG were (until recently) always assumed to be in head coordinates.

I see two options really:

1. A bigger-harder change is to be smarter about it now and do some UNKNOWN coord frame stuff in MNE-Python and here.
2. A smaller and quicker (but less correct) change is just to detect this head-frame-but-no-fiducials case for EEG-only data and strip the info['dig'] EEG points. This would be equivalent to behavior in MNE-Python 1.1. This would at least allow us to decide to do something smarter later.

My vote is to do (2) for now since MNE-Python 1.2 release is in the next week. Then we can take weeks/months to sort out the correct solution via (1) in follow-up PRs.

[sappelhoff]

I think we can merge this and deal with the dig issue in a separate PR.

Thanks @larsoner

[agramfort]

+1

…

Message ID: ***@***.***>