MAINT: Use mffpy for EGI metadata reading and add PNS shim#13945
MAINT: Use mffpy for EGI metadata reading and add PNS shim#13945PragnyaKhandelwal wants to merge 6 commits into
Conversation
PragnyaKhandelwal
changed the title
PragnyaKhandelwal
requested review from
agramfort,
drammock and
larsoner
as code owners
|
Hey @scott-huberty! This one's ready for a look when you get a chance. Swapped out |
…e merged upstream fix BEL-Public/mffpy#145
for more information, see https://pre-commit.ci
PragnyaKhandelwal
force-pushed
the
egi-mff-metadata
branch
from
823381d to
510e0fa
Compare
scott-huberty
left a comment
scott-huberty
left a comment
There was a problem hiding this comment.
LGTM! I have a few nitpicks, please see my comments.
| if " " in record_time and "T" not in record_time: | ||
| record_time = record_time.replace(" ", "T") |
There was a problem hiding this comment.
None of our test files will hit this line. Do we need it? Does this check exist on main?
| if name == "None": | ||
| name = "" |
There was a problem hiding this comment.
This is not hit by our tests. Do we need it? AFAICT, name == "None" would hit the else clause below on L178. Do we need this?
| num_str = str(sensor.get("number", "")) | ||
|
|
||
| if not name: | ||
| name = channel_naming % int(num_str) if num_str else "Unknown" |
There was a problem hiding this comment.
In MNE-Python, channel names must be unique. If name == None for more than 1 channel, then they would both be assigned name = 'Unknown', which will raise an error later, probably during mne.create_info.
2 participants
Reference issue (if any)
None
What does this implement/fix?
This PR completes the refactor of the EGI MFF reader to utilize the native
mffpylibrary for metadata parsing, replacing the legacydefusedxmlimplementation.Key changes include:
info.xml,sensorLayout.xml,pnsSet.xml, andcoordinates.xmltomffpy.xml_files.XML.from_file().mne/fixes.pyto handlePNSSetsensor parsing, bypassing aKeyError: 'conversion'found inmffpyv0.11.0 whenpnsSet.xmlcontains a<conversion>tag.mne/io/egi/egimff.pyto ensure consistent channel naming and namespace handling, resolvingRuntimeWarning: Channel names are not uniqueerrors.mffpydependency to>= 0.11.0inpyproject.tomlandenvironment.yml.Dependencies
mffpydependency to>= 0.11.0inpyproject.tomlandenvironment.yml.KeyError: 'conversion'and nanosecond timestamp parsing issues encountered when usingmffpynatively.mffpyfor EGI MFF event reading #13932.Additional information
I have reported the upstream
mffpyissue here: mffpy/#144. We are using a temporary fallback shim inmne/fixes.pyto keep the MNE test suite stable while waiting for an upstream fix.