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

FIX : read fids in eeglab #10521

Merged
merged 3 commits into from
Apr 14, 2022
Merged

FIX : read fids in eeglab #10521

merged 3 commits into from
Apr 14, 2022

Conversation

agramfort
Copy link
Member

closes #10474

@cmadjar this should fix your problem.

@hoechenberger
Copy link
Member

SphinxWindows failure is unrelated

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

nitpick, otherwise LGTM!

mne/io/eeglab/eeglab.py Outdated Show resolved Hide resolved
Comment on lines +466 to +468
assert pos['nasion'] is not None
assert pos['lpa'] is not None
assert pos['rpa'] is not None
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe check whether the arrays have the correct dimension too?

@hoechenberger
Copy link
Member

Is the coreg failure related? (I don't think so?)
If not, feel free to merge!

@agramfort
Copy link
Member Author

@larsoner no idea about these CIs failures. @larsoner merge if happy

@larsoner larsoner merged commit 5e60e47 into mne-tools:main Apr 14, 2022
@larsoner
Copy link
Member

Thanks @agramfort

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

You guys are so fast. I only got the chance to test the PR today. I confirm that reading the fids works now for our SET file. 👍

However, I do not see them being written again in the SET file when saving the data again and opening them in EEGLAB. If I reopen the new file with read_raw_eeglab, I can confirm that the fids are not there either. @agramfort should I open a different issue for that?

@larsoner
Copy link
Member

should I open a different issue for that?

Sure, that's an exporting rather than reading bug. Or you can reopen your old issue an re-title it to discuss round-trip failing. (Now the read works, but the write doesn't.)

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

See #10529 for the new issue. I linked the old issue in the description as well.

@agramfort
Copy link
Member Author

agramfort commented Apr 14, 2022 via email

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

We need to create a BIDS structure with the SET file after anonymization of other fields in order to share data with others.

@hoechenberger
Copy link
Member

Couldn't EEGLAB's BIDS export be an option for you, then?

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

Not really unfortunately as it requires a Matlab license...

@hoechenberger
Copy link
Member

hoechenberger commented Apr 14, 2022

And if you try our BrainVision export? Would that maybe help? Maybe it works better :)

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

our collaborator wants SET files... We have no other choice than to have the mne export functionality to work with SET files. Sorry

@agramfort
Copy link
Member Author

@cmadjar you need to report this issue here https://github.com/jackz314/eeglabio/issues

we use this code to export

@cmadjar
Copy link

cmadjar commented Apr 14, 2022

ok. Done!

larsoner added a commit to wmvanvliet/mne-python that referenced this pull request Apr 19, 2022
* upstream/main: (40 commits)
  FIX: Flake (mne-tools#10540)
  FIX: Correct link (mne-tools#10536)
  DOC: Update installers (mne-tools#10535)
  ENH: Add dark mode to website (mne-tools#10523)
  WIP: Copy BEM surfaces by default (don't symlink) (mne-tools#10531)
  Avoid lowpass=0 in brainvision data (mne-tools#10517)
  DOC: Update installers [skip azp] [skip actions] (mne-tools#10528)
  FIX: Fix for old build (mne-tools#10527)
  Fix line noise at wrong frequencies (mne-tools#10525)
  FIX : read fids in eeglab (mne-tools#10521)
  MAINT: Prefer PySide6 in testing (mne-tools#10513)
  ENH: Add overview_mode support (mne-tools#10501)
  MRG: Updates for qtpy in mne-qt-browser (mne-tools#10509)
  BUG: Fix bug with themes on macOS (mne-tools#10500)
  MAINT: Bump installer links (mne-tools#10511)
  Add metadata to combine_channels (mne-tools#10504)
  MAINT: Standardize tests (mne-tools#10502)
  CI: Test circle (mne-tools#10506)
  ENH: Use HiDPI splash screen on HiDPI screens (mne-tools#10503)
  WIP,MNT: Add support for QtPy (mne-tools#10430)
  ...
larsoner added a commit to agramfort/mne-python that referenced this pull request Apr 21, 2022
* upstream/main: (52 commits)
  MAINT: Extra test for coreg (mne-tools#10549)
  BUG: Fix annot meas_date / crop (mne-tools#10491)
  Update latest.inc
  Use liblinear solver instead of lbgfs in all decodung examples [skip azp][skip actions] (mne-tools#10552)
  STY: Hotfix for HTML [ci skip]
  Allow making inverse solutions from fixed-orientation discrete forward models, enabling multi-dipole modeling (mne-tools#10464)
  Correct documented default number of CV splits [skip azp][skip actions] (mne-tools#10548)
  MRG: Add error checking to prevent the creation of montage with invalid [x, y, z] (mne-tools#10547)
  MRG: Add "verbose" parameter to pick_channels() method (mne-tools#10544)
  CI: Avoid bad PySide6 (mne-tools#10545)
  FIX: Flake (mne-tools#10540)
  FIX: Correct link (mne-tools#10536)
  DOC: Update installers (mne-tools#10535)
  ENH: Add dark mode to website (mne-tools#10523)
  WIP: Copy BEM surfaces by default (don't symlink) (mne-tools#10531)
  Avoid lowpass=0 in brainvision data (mne-tools#10517)
  DOC: Update installers [skip azp] [skip actions] (mne-tools#10528)
  FIX: Fix for old build (mne-tools#10527)
  Fix line noise at wrong frequencies (mne-tools#10525)
  FIX : read fids in eeglab (mne-tools#10521)
  ...
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.

read_raw_eeglab does not read nasion, lpa and rpa values from .set file
4 participants