Skip to content

Fix file format check in _check_eeglab_fname function#12523

Merged
larsoner merged 5 commits intomne-tools:mainfrom
neuromechanist:main
Apr 2, 2024
Merged

Fix file format check in _check_eeglab_fname function#12523
larsoner merged 5 commits intomne-tools:mainfrom
neuromechanist:main

Conversation

@neuromechanist
Copy link
Copy Markdown
Contributor

@neuromechanist neuromechanist commented Apr 1, 2024 •
edited
Loading

Fixes #12500

Following the conversation in the issue, it seems that the explicit check for the .fdt and raising error is not necessary.

By removing the Error condition, the method will look for a .fdt file in the directory if it does not find it in the EEG.data field while issuing a warning. This is also the expected and observed behavior in EEGLAB.

I have tested the behavior with the GDrive file shared in the issue, which has stemmed from OpenNeuro dataset ds003645.

CC: @arnodelorme
Thanks.

@welcome
Copy link
Copy Markdown

welcome bot commented Apr 1, 2024

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

larsoner
larsoner reviewed Apr 1, 2024
View reviewed changes
larsoner
larsoner reviewed Apr 1, 2024
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.

Does this also implement the suggestion:

That said, checking for the presence of an .fdt with the same base filename as the .set does seem sensible / fairly safe, so if you're up for making a pull request I don't really object to adding that behavior.

Or no?

mne/io/eeglab/eeglab.py
Comment on lines -55 to -56
elif fmt != ".fdt":
raise OSError("Expected .fdt file format. Found %s format" % fmt)
Copy link
Copy Markdown
Member

@larsoner larsoner Apr 1, 2024

Choose a reason for hiding this comment

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

If .fdt is standard, it's perhaps better to change this to a warn(...) rather than remove it entirely

Copy link
Copy Markdown
Contributor Author

@neuromechanist neuromechanist Apr 1, 2024

Choose a reason for hiding this comment

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

Yes, .fdt is the standard when the data is not included in the EEG structure.
However there is a warning already in place a few lines down (line 69) if the .fdt file is not found, which already covers 1) there is no .fdt file, or 2) the .fdt file name is incorrect.

So, I did not add another warning to avoid issuing double warnings for a single issue.

@neuromechanist @larsoner
Update doc/changes/devel/12523.bugfix.rst
b84f7f9 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@neuromechanist
Copy link
Copy Markdown
Contributor Author

neuromechanist commented Apr 1, 2024

Does this also implement the suggestion:

That said, checking for the presence of an .fdt with the same base filename as the .set does seem sensible / fairly safe, so if you're up for making a pull request I don't really object to adding that behavior.

Or no?

I believe this has been already addressed in the current implementation (Lines56-70)

@drammock
Copy link
Copy Markdown
Member

drammock commented Apr 2, 2024

Does this also implement the suggestion:

That said, checking for the presence of an .fdt with the same base filename as the .set does seem sensible / fairly safe, so if you're up for making a pull request I don't really object to adding that behavior.

Or no?

I believe this has been already addressed in the current implementation (Lines56-70)

The lines you're linking to don't show up in the "files changed" tab of this pull request.

@neuromechanist
Copy link
Copy Markdown
Contributor Author

neuromechanist commented Apr 2, 2024

Does this also implement the suggestion:

That said, checking for the presence of an .fdt with the same base filename as the .set does seem sensible / fairly safe, so if you're up for making a pull request I don't really object to adding that behavior.

Or no?

I believe this has been already addressed in the current implementation (Lines56-70)

The lines you're linking to don't show up in the "files changed" tab of this pull request.

Yes, I could not link to these lines in the blame because they have not changed. They are just below the changed (deleted) lines.

@drammock
Copy link
Copy Markdown
Member

drammock commented Apr 2, 2024

Yes, I could not link to these lines in the blame because they have not changed. They are just below the changed (deleted) lines.

ah, ok sorry! I was confused because you linked to the lines in your fork not in current main, so I (wrongly) thought you were saying "I already implemented this here". I didn't realize that was in main already and just wasn't being reached because the error came first.

@neuromechanist
Copy link
Copy Markdown
Contributor Author

neuromechanist commented Apr 2, 2024

@drammock, yes, that is precisely the case. Sorry for the confusion.

@drammock
Copy link
Copy Markdown
Member

drammock commented Apr 2, 2024

by the way, you can "expand down" the diff of any files you changed (using the down-arrow button below the line numbers), and then link to unchanged lines. like this: https://github.com/mne-tools/mne-python/pull/12523/files#diff-9757a570dcb234f7b79ea08fa0145a3376617094785146136095147b32f0dd45L64-L68

(note that the URL ends with L64-L68 now)

@larsoner larsoner merged commit a02838f into mne-tools:main Apr 2, 2024
@welcome
Copy link
Copy Markdown

welcome bot commented Apr 2, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@larsoner
Copy link
Copy Markdown
Member

larsoner commented Apr 2, 2024

Thanks @neuromechanist !

larsoner added a commit to larsoner/mne-python that referenced this pull request Apr 15, 2024
@larsoner
Merge remote-tracking branch 'upstream/main' into rank
455a3dc 
* upstream/main: (50 commits)
  ENH: Improve OPM auditory dataset and example (mne-tools#12539)
  MAINT: Bump to latest pydata-sphinx-theme (mne-tools#12228)
  MRG: Simplify manual installation instructions a little by dropping explicit mention of (lib)mamba (mne-tools#12362)
  fix PSD weights handling when bad annotations present (mne-tools#12538)
  Fix phase loading (mne-tools#12537)
  align FFT windows to good data spans in psd_array_welch (mne-tools#12536)
  explicitly disallow multitaper in presence of bad annotations (mne-tools#12535)
  MAINT: Clean up PyVista contexts (mne-tools#12533)
  MAINT: Complete API change of ordered (mne-tools#12534)
  MAINT: Reinstall statsmodels and improve logging (mne-tools#12532)
  MAINT: Remove scipy.signal.morlet2 (mne-tools#12531)
  Update README badge links (mne-tools#12529)
  BUG: Fix bug with reading his_id from snirf (mne-tools#12526)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12524)
  Fix file format check in _check_eeglab_fname function (mne-tools#12523)
  MAINT: Reenable picard in pre testing (mne-tools#12525)
  MAINT: Bump to large resource class (mne-tools#12522)
  MAINT: Restore 2 jobs on Windows (mne-tools#12520)
  Add exclude_after_unique option to mne.io.read_raw_edf/read_raw_edf to search for exclude channels after making channel names unique (mne-tools#12518)
  Improve consistency of sensor types in code and documentation (mne-tools#12509)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner left review comments

@drammock drammock drammock approved these changes

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reduce Expected .fdt file format error to warning

4 participants

@neuromechanist @drammock @larsoner @hoechenberger