EGI: stabilize mffpy-backed MFF reader#13914
Conversation
- more functional - annotate acquisition skips
for more information, see https://pre-commit.ci
|
Hey @scott-huberty and @drammock! Here is the fresh branch for Phase 1 (superseding #13684). |
|
Thanks @PragnyaKhandelwal ! Can you please open a meta-issue on this repo that is titled something like "GSoC 2026: Use mffpy for EGI Reader"? Then you can include one checkbox for each milestone. This can mirror the proposal plan you submitted, but does not need to be as detailed, it can be something like:
This will help us keep track of the project's progress. Does that work for you? |
Thanks @scott-huberty! I have opened the tracking meta-issue here: #13926 |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
I left a few specific comments in-line, but the overall high-level comments:
- I was expecting this first PR to be mostly about the tests (google doc mentioned there were cross-platform inconsistencies in the tests, and purging legacy helper funcs from the tests) but there's a lot of changed lines in the package code too.
- the diff is big. I suggest breaking the problem into smaller pieces and doing multiple PRs.
- It's worth considering an approach where very small pieces of functionality are done in each PR (to keep diffs small / easy to review). For example, one PR might use mffpy to read events (but use legacy code for everything else); the next might use mffpy to also read metadata, and a third might use it to read the actual data samples.
Let's try to work out a more detailed plan at the meeting.
| except (ValueError, AttributeError): | ||
| # mffpy has a bug parsing timestamps with 9 decimal places (nanoseconds) | ||
| # Workaround: manually parse the timestamp from the info.xml file | ||
| import xml.etree.ElementTree as ET |
| try: | ||
| return mff_reader.startdatetime | ||
| except (ValueError, AttributeError): | ||
| # mffpy has a bug parsing timestamps with 9 decimal places (nanoseconds) |
There was a problem hiding this comment.
- did you open an issue upstream yet?
- normally when we need to shim upstream code to work around bugs, we put it in
mne/fixes.pyand import the function from there. The shim function should have a comment likeTODO VERSION ...explaining / linking to upstream issues/PRs so we can easily see when it's OK to remove the shim.
| return datetime.datetime.strptime(time_str, "%Y-%m-%dT%H:%M:%S.%f%z") | ||
|
|
||
|
|
||
| def _parse_egi_datetime(time_str): |
There was a problem hiding this comment.
same comment: shims go in mne/fixes.py
| fname = op.join(filepath, "coordinates.xml") | ||
| if not op.exists(fname): | ||
| warn("File coordinates.xml not found, not setting channel locations") | ||
| logger.warn("File coordinates.xml not found, not setting channel locations") |
There was a problem hiding this comment.
revert. We have a wrapper that does some clever magic with e.g. the stack level, so that tracebacks are easier to read
| raise RuntimeError( | ||
| f"Number of defined channels ({n_chans}) did not match the " | ||
| f"expected channels ({summaryinfo['n_channels']})." | ||
| f"Number of defined channels ({n_chans}) did not match the expected " |
There was a problem hiding this comment.
try to avoid unrelated / cosmetic changes (to keep the diff small). If you must make them, do them in a separate commit (or even separate PR) --- again, for easier review
3 participants
Reference issue (if any)
What does this implement/fix?
main-based branch.mffpybackend integration.main.Additional information
1 passed, 23 skipped.AI assistance