Skip to content

MRG: Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader#9990

Merged
larsoner merged 11 commits intomne-tools:mainfrom
mscheltienne:eeglab_reader_add_support_for_ch_types
Nov 10, 2021
Merged

MRG: Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader#9990
larsoner merged 11 commits intomne-tools:mainfrom
mscheltienne:eeglab_reader_add_support_for_ch_types

Conversation

@mscheltienne
Copy link
Copy Markdown
Member

@mscheltienne mscheltienne commented Nov 10, 2021 •
edited
Loading

Following #9989, a quick proposition to read channel types with EEGLAB reader.

In the reader, the data is read in a variable named eeg since EEGLAB is mostly focused on EEG datasets. However, in practice it does support more data types; and I could rename left and right this variable to e.g. data to make the code a bit more general if you want.

@mscheltienne mscheltienne mentioned this pull request Nov 10, 2021
Open
3 tasks
@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Nov 10, 2021

I think a test would be really useful here. Can you use an existing .set file and patch the stuff that needs to be adapted on the fly?

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021 •
edited
Loading

@cbrnr Oh right.. I'm dumb, I can just edit the ['chanlocs']['type'] from the loaded file on the fly.. I'll add it.

@cbrnr cbrnr changed the title Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader. Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader Nov 10, 2021
@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Nov 10, 2021

@mscheltienne in case you're wondering why I edited the title, you don't have to include a period (super nitpicky I know).

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021

Ahah alright, I was looking at it and had no idea what changed!

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021 •
edited
Loading

So.. it's actually a pain to test for now. The test I added is restricted to the modification to the private function _get_montage_information in the end.
IMO, a complete I/O roundtrip test FIF -> SET -> FIF requires either a different .set file in the testing dataset (to test SET -> FIF) OR the addition of jackz314/eeglabio#4

By the way, I did test with the eeglabio PR branch the complete roundtrip FIF -> SET -> FIF, and both EEGLAB and MNE displayed the correct channel types.

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Nov 10, 2021

Ready for review/merge @mscheltienne ?

larsoner
larsoner reviewed Nov 10, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Other than a simplification idea, LGTM!

larsoner
larsoner requested changes Nov 10, 2021
View reviewed changes
Copy link
Copy Markdown
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.

... oh actually you need a latest.inc update. I think this should qualify as a bugfix, as 1) it fixes type handling and 2) people's code could actually break from this PR if they've been picking types as 'eeg' that will now be something else

@mscheltienne @larsoner
Update mne/io/eeglab/eeglab.py [ci skip]
5c5c542 
Tested locally.

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
larsoner
larsoner reviewed Nov 10, 2021
View reviewed changes
@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021

I don't know why the doc build seems to fail.. Any idea?

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Nov 10, 2021

It's pydata/pydata-sphinx-theme#508, I'll open a PR to fix it

@larsoner larsoner mentioned this pull request Nov 10, 2021
Merged
@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021 •
edited
Loading

@larsoner Any particular reason why read_epochs_eeglab lives in the mne. namespace?

https://github.com/mne-tools/mne-python/blob/main/mne/__init__.py#L34

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021

Same remark for read_epochs_kit one line above.

@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021

And for from .io import read_epochs_fieldtrip, read_evoked_fieldtrip, read_evokeds_mff line 100.

@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Nov 10, 2021

This discussion sounds familiar 😄

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Nov 10, 2021

Discussion in #9560

larsoner
larsoner reviewed Nov 10, 2021
View reviewed changes
@mscheltienne @larsoner
Update doc/changes/latest.inc [skip azp] [skip actions]
a1fe9c1 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@mscheltienne
Copy link
Copy Markdown
Member Author

mscheltienne commented Nov 10, 2021 •
edited
Loading

Alright, I didn't read it entirely last time I saw it; and I didn't remember it also mentioned the read_epochs methods. So io is only for raw instance I/O because Raw lives in mne.io.. at least I'll know it now.

@mscheltienne mscheltienne changed the title Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader MRG: Add support to retrieve channel type from ['chanlocs']['type'] with EEGLAB reader Nov 10, 2021
@larsoner larsoner merged commit 7491370 into mne-tools:main Nov 10, 2021
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Nov 10, 2021

Thanks @mscheltienne !

@mscheltienne mscheltienne deleted the eeglab_reader_add_support_for_ch_types branch November 10, 2021 21:50
larsoner added a commit that referenced this pull request Nov 10, 2021
@mscheltienne @larsoner
Add support to retrieve channel type from ['chanlocs']['type'] with E…
9088f53 
…EGLAB reader (#9990)

* Add support to retrieve channel type from ['chanlocs']['type'].

* Add small test of _get_montage_info

* fix flake.

* Remove one line.

* Update mne/io/eeglab/eeglab.py [ci skip]

Tested locally.

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Add entry to changelog [skip azp] [skip actions]

* Add warning and test.

* Add more detailed warning.

* fix flake8

* style commit to rerun doc build [skip azp] [skip actions]

* Update doc/changes/latest.inc [skip azp] [skip actions]

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@mscheltienne mscheltienne mentioned this pull request Nov 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mscheltienne @cbrnr @larsoner