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.

Sign up for GitHub

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

Jump to bottom

Fix file format check in _check_eeglab_fname function #12523

Merged
merged 5 commits into from
Apr 2, 2024
Merged

Fix file format check in _check_eeglab_fname function #12523

merged 5 commits into from
Apr 2, 2024

Conversation

neuromechanist
Copy link
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 Welcome
Copy link

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
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.

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
Member

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
Contributor Author

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
Copy link
Contributor Author

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
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
Contributor Author

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
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
Contributor Author

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

@drammock
Copy link
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
30 checks passed
@welcome Welcome
Copy link

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
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 dengemann is a code owner

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