-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[MRG] Coregistration-GUI: use *.mff as digitization source #8790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of supporting formats other than FIF! I think we can probably make it general to all formats rather than just extending to EGI that should take about the same number of lines. I think that there are other formats (e.g., CTF) that would immediately benefit. Feel free to pytest.mark.parametrize over these files in a test to see if you want.
mne/gui/_file_traits.py
Outdated
| if self.file: | ||
| if not self.file: | ||
| return | ||
| elif self.file.endswith('.mff'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make the pattern self.file.endswith('.fif') or self.file.endswith('.fif.gz'): and in that case use return read_info(...), else use return read_raw(...).info. This should make it work with any reader that supports digitization
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one small idea for more DRY code and needing to wait for the EGI PR then a rebase/merge here, LGTM! Also don't forget to update doc/changes/latest.inc
mne/gui/_file_traits.py
Outdated
| for d in info['dig']: | ||
| if d['kind'] == FIFF.FIFFV_POINT_CARDINAL: | ||
| has_point[d['ident']] = True | ||
| if not all(has_point.values()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this whole section could be simplified, DRY'd, and have fewer missed coverage lines with something like:
missing = [key for key in ('LPA', 'Nasion', 'RPA') if not
has_point[getattr(FIFF, f'FIFFV_POINT_{key.upper()}']
if missing:
points = _fiducial_coords(info['dig'])
if len(points == 3):
_append_fiducials(info['dig'], *points.T)
else:
error(...)
self.reset_traits(['file'])
return
1d8609a to
1f633ac
Compare
|
I added tests for MFF and CTF, are there other formats that contain digitizer info? |
|
CI failures look unrelated, thanks @christianbrodbeck ! |
* upstream/main: MRG, ENH: Add warning about bad whitener conditioning (mne-tools#8805) MRG, MAINT: Deprecated param and pytest-qt (mne-tools#8808) fix mne.viz.plot_topomap with some missing grad in a pair (mne-tools#8817) [MRG] Coregistration-GUI: use *.mff as digitization source (mne-tools#8790) FIX: Path [MRG] ENH EGI MFF reader: populate info['dig'] (mne-tools#8789) MRG: Improve Brain UX (mne-tools#8792) FIX missing Axes3D import in viz._3d._plot_mpl_stc (mne-tools#8811) Better error message if configured download folder doesn't exist (mne-tools#8809) MRG, ENH: Add support for other formats to browse_raw (mne-tools#8807) MRG, BUG: Allow depth > 1 (mne-tools#8804)
Depends on #8789.
This adds the capability to use
*.mfffiles directly as digitization source to the coregistration GUI. I started on it for verifying the result of #8789, but it could be a useful feature since it obviates the need to convert to FIFF before doing coregistration. The screen-shot istest_egi.mff.