-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[MRG] ENH EGI MFF reader: populate info['dig'] #8789
Conversation
67da2fd
to
f46ed0c
Compare
mne/io/egi/tests/test_egi.py
Outdated
@@ -115,6 +115,11 @@ def test_io_egi_mff(): | |||
test_scaling=False, # XXX probably some bug | |||
) | |||
assert raw.info['sfreq'] == 1000. | |||
assert len(raw.info['dig']) == 132 # 1 ref + 128 eeg + 3 cardinal points | |||
assert raw.info['dig'][0]['ident'] == 129 # reference channel nr |
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.
reference channel usually gets ident=0, then all other channels start from 1
>>> import mne
>>> info = mne.io.read_info(mne.datasets.sample.data_path() + '/MEG/sample/sample_audvis_raw.fif')
>>> info['dig'][85]
<DigPoint | EEG #0 : (2.4, 111.0, -35.0) mm : head frame>
>>> info['dig'][86]
<DigPoint | EEG #1 : (-37.4, 105.7, 73.3) mm : head frame>
>>> info['chs'][315]['ch_name']
'EEG 001'
>>> np.round(info['chs'][315]['loc'][:6] * 1000, 1)
array([-37.4, 105.7, 73.3, 2.4, 111. , -35. ])
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.
Hm, so that would mean losing the EGI channel numbering -- is that preferable? I'm not sure, but couldn't there be a situation where you'd be looking at the locations and wanted to know which channel that was? Or would you always be looking at the info['chs']
when this is relevant?
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.e., seeing <DigPoint | EEG #0 : (2.4, 111.0, -35.0) mm : head frame>
might be misleading if it does not correspond to the channel named E0
...
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.
Yes the idea is that we take each format and make it conform to the info
spec as much as possible. If for some reason we need the original ordering then we should store it in a public attribute or something.
One more point is that the EEG dig
entries with idents 1...N
should be in the same order as the EEG channels in the data/Raw instance. But maybe you have it this way already.
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.
Yes it's an unfortunate coincidence that they start their numbering at zero and this collides with our dig repr
which always says EEG #N
starting from zero, but I think we should live with it and not deviate from our standard behavior here
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.
One more point is that the EEG dig entries with idents 1...N should be in the same order as the EEG channels in the data/Raw instance. But maybe you have it this way already.
This is the case with the exception that the reference seems to be always the last EEG channel in the EGI format (as well as the mne
objects created by the current importer).
So did I understand correctly that we want to move the reference and call it #0 in dig
but leave it last in the data/chs
?
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.
Oh it actually shows up as a channel in the data, too (just with necessarily zero value)? Then I guess in that case it's okay to leave it as is. The alternative would be to duplicate the point but I don't think that's a good solution. We can reconfigure later if need be
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.
Yes it's in the data with all 0s.
In [2]: raw = mne.io.read_raw('test_egi.mff')
In [5]: raw.ch_names.index('E129')
Out[5]: 128
In [10]: numpy.all(raw._data[128] == 0)
Out[10]: True
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)
Reference issue
Partly addresses #8038
What does this implement/fix?
Adds digitization points to
.info
forread_raw_egi
andread_evokeds_mff