BUG: don't assume that channel info contains particular keys #11074
Conversation
- should fix regression introduced in 5e60e47
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
|
was just discussing this yesterday (with @larsoner and @agramfort). We thought it best to treat the unidentified fiducials as regular headshape points (rather than ignoring them entirely) and add a warning that this was being done. |
|
I don't understand everything in that sentence 😉 But sure, as long as it doesn't crash any solution is fine with me. I'll take a look at #11039, which had escaped my attention. Do you want to close this PR? |
|
Sorry, I dashed off my reply in a hurry. No need to close this PR, it's 90% of the way there. What I meant was: the fields in question are fiducials ("fids"), AKA cardinal anatomical landmarks. "Headshape points" are other digitized points located elsewhere on the scalp or face. Together they are all used to coregister electrode locations with an MRI image. If the fiducials are present in an EEGLAB file, but their description is empty (so we don't know for certain which is left/right ear (LPA/RPA) and which is nose (NAS), then instead of simply discarding the information, we can/should treat them as regular (unlabelled) headshape points. However, looking at the (side note: it seems like it ought to be possible to simply infer from the XYZ values which of the fids is which. @agramfort / @larsoner WDYT about, e.g., inferring that RPA is the fid point with the highest X value, NAS is the one with the highest Y value, and LPA is the other one? Can we safely assume the pts are in head coord frame?) |
Unfortunately it's unsafe because we can't assume the points are near the head coordinate frame. They probably usually are, but sometimes might be quite far from it
I think it would be nice to fix it here. Feel free to give it a shot if you're up for it @smathot , or I can push some commits if you want help |
|
@smathot just to close the loop: @larsoner, @agramfort and I discussed in person and realized that past behavior before #10521 was to ignore these points, so we'll continue to ignore them now (i.e., not try to treat as headshape points). Hence no further changes needed here. |
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
|
Thanks for handling this so quickly! 🥳 |
* upstream/main: (366 commits) BUG: Spectrum deprecation cleanup [circle deploy] (mne-tools#11115) Add API entry list and map (mne-tools#10999) Add legacy decorator (mne-tools#11097) [ENH, MRG] Add time-frequency epoch source estimation (mne-tools#11095) Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104) MRG: Fixes for mne-tools#11090 (mne-tools#11108) add test for edf units param (mne-tools#11105) BUG: Improve logic for bti (mne-tools#11102) add spectrum class (mne-tools#10184) ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099) ENH: Add temperature and galvanic (mne-tools#11090) Add error message when conversion of EEG locs to head space fails (mne-tools#11080) DOC: removed unnecessary line in PSF example (mne-tools#11085) FIX: Update helmet during ICP (mne-tools#11084) Fix various typos (mne-tools#11086) DOC: Rel BUG: don't assume that channel info contains particular keys (mne-tools#11074) [BUG] Fix plot_topomap with sphere="eeglab" (mne-tools#11081) Add `vmin` and `vmax` specification to `mne.Evoked.animate_topomap` (mne-tools#11073) Add _on_missing functionality to UpdateChannelsMixin (mne-tools#11077) ...
What does this implement/fix?
Load a specific
.setfile broke in the latest release of MNE. The reason is a regression introduced along with 5e60e47, which assumes that certain fields exist, thus resulting in a crash when they don't. It's possible that my.setis non-standard, I'm not familiar enough with the format to be able to say that, but this PR seems safe enough in any case.closes #11039