Skip to content
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

use channel names in read_raw_egi and set the reference location #10898

Merged
merged 14 commits into from
Jul 21, 2022

Conversation

scott-huberty
Copy link
Contributor

closes #10869 (supersedes)
closes #10782

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failures look real, let me know if you need help or want me to take a deeper look at the code

mne/io/egi/egi.py Outdated Show resolved Hide resolved
@scott-huberty
Copy link
Contributor Author

CI failures look real, let me know if you need help or want me to take a deeper look at the code

Hey @larsoner super sorry for the belated reply, I was in an all-day intensive summer course last week that runs through this week - once it is finished I'll have the bandwidth to return to this, I hope that's alright!
And thanks for your suggestion, I'll give the code a try. And just let me know if you'd like me to convert this PR back into a draft.

BTW, you may have noticed that the approach to this PR changed since our last paired programming session - where the initial idea was to make the new channel naming scheme an option that could be turned on by a parameter in the read_raw_egi function call. After a long discussion, we changed course a bit.

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scott-huberty I know you're busy this week, just leaving this review for when you're ready to jump back in. LMK if you want to schedule a pair-programming session to work on this.

One other thing we'll need here is to add a changelog entry (docs/changes/latest.inc) to the "bugfix" section (IIRC @agramfort views this as a bug that we were previously ignoring the name field, and I agree). You'll also need to add yourself do docs/changes/names.inc if you're not there already.

mne/io/egi/egimff.py Show resolved Hide resolved
mne/io/egi/egimff.py Outdated Show resolved Hide resolved
mne/io/egi/egimff.py Outdated Show resolved Hide resolved
mne/io/egi/egimff.py Outdated Show resolved Hide resolved
mne/io/egi/egimff.py Outdated Show resolved Hide resolved
mne/io/egi/egimff.py Outdated Show resolved Hide resolved
mne/io/egi/tests/test_egi.py Outdated Show resolved Hide resolved
mne/io/egi/tests/test_egi.py Outdated Show resolved Hide resolved
mne/io/egi/tests/test_egi.py Outdated Show resolved Hide resolved
@scott-huberty
Copy link
Contributor Author

@scott-huberty I know you're busy this week, just leaving this review for when you're ready to jump back in. LMK if you want to schedule a pair-programming session to work on this.

One other thing we'll need here is to add a changelog entry (docs/changes/latest.inc) to the "bugfix" section (IIRC @agramfort views this as a bug that we were previously ignoring the name field, and I agree). You'll also need to add yourself do docs/changes/names.inc if you're not there already.

Hey @drammock, i'm back in office and looking over your suggestions now. A paired programming session would be really helpful, hopefully we will be able to address the remaining issues and close this!

I am fairly open this week, let me know if there is a certain time that works best for you.

@drammock
Copy link
Member

I'll ping you on discord to schedule the pair programming

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on pair-programming session w/ @scott-huberty. +1 for merge when CIs are green

doc/changes/latest.inc Outdated Show resolved Hide resolved
@drammock
Copy link
Member

CI failures will be fixed by #10944

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When reading EGI data (.mff), import channel names from sensorLayout.xml
3 participants