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

[MRG]: Add support for reading curry EEG files #1072

Merged
merged 9 commits into from
Oct 1, 2022

Conversation

dengemann
Copy link
Member

@dengemann dengemann commented Sep 29, 2022

PR Description

This adds support for curry eeg files as input, closes #1070.
On local machine, not all tests pass on main branch, hence, I am not sure what certain test results mean for curry branch.
Let's see what CI's will say.

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • [x ] PR description includes phrase "closes <#issue-number>"

@welcome
Copy link

welcome bot commented Sep 29, 2022

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

@dengemann dengemann changed the title Curry [ENH]: Add support for reading curry EEG files Sep 29, 2022
@hoechenberger
Copy link
Member

Merging main just to be safe 😇

@hoechenberger
Copy link
Member

@dengemann it appears the .dat extension is already in use for another reader.. maybe let's restrict ourselves to .cdt here?

@dengemann
Copy link
Member Author

@dengemann it appears the .dat extension is already in use for another reader.. maybe let's restrict ourselves to .cdt here?

OK, that would move curry support to version > 7

Or does it mean that more sophisticated checks are needed?

@dengemann
Copy link
Member Author

also we did not updated whats new etc.

@hoechenberger
Copy link
Member

@sappelhoff We have a one-to-many mapping here, .dat can mean multiple file formats. What should we do?

@dengemann
Copy link
Member Author

dengemann commented Sep 29, 2022

... perhaps we don't support it for now and wait till someone asks for it. I am personally OK with .cdt support as it solves my problem.

@agramfort
Copy link
Member

agramfort commented Sep 29, 2022 via email

@hoechenberger
Copy link
Member

+1, let's only do .cdt for now then!!

@dengemann
Copy link
Member Author

shall I open the PR again / continue or will you take over @hoechenberger ?

@hoechenberger
Copy link
Member

Please do work on this one as far / long as you can, @dengemann :) I don't have much time currently

@larsoner
Copy link
Member

#1073 pushes commits that xfail the unrelated tests so that hopefully the feedback here will be more meaningful. You might want to git fetch upstream && git merge upstream/main after it's merged so that you can benefit from the dataset caching, too

@hoechenberger
Copy link
Member

Pulled in main so we can see what's still failing here, as suggested by @larsoner

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #1072 (14a322e) into main (1a50295) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1072   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          24       24           
  Lines        3845     3845           
=======================================
  Hits         3661     3661           
  Misses        184      184           
Impacted Files Coverage Δ
mne_bids/config.py 97.64% <ø> (ø)
mne_bids/read.py 95.95% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@agramfort
Copy link
Member

@dengemann can you look into CI failures?

@dengemann
Copy link
Member Author

Alright, made some progress here. CIs happy except for coverage.

@dengemann
Copy link
Member Author

Ok it semes all is fine now.

@@ -59,7 +59,8 @@
'.lay': 'Persyst', '.dat': 'Persyst',
'.EEG': 'Nihon Kohden',
'.cnt': 'Neuroscan', '.CNT': 'Neuroscan',
'.bin': 'EGI'}
'.bin': 'EGI',
'.cdt': 'n/a'}
Copy link
Member

Choose a reason for hiding this comment

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

Why not "Curry"?

Copy link
Member Author

Choose a reason for hiding this comment

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

in my understanding Curry is used by different amps / manufacturers

Copy link
Member Author

Choose a reason for hiding this comment

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

ah sorry. got confused. yes we could do "curry" for the fileformat

@@ -24,7 +24,7 @@ Version 0.11 (unreleased)

The following authors contributed for the first time. Thank you so much! 🤩

* ...
* `Denis Engemann`_
Copy link
Member

Choose a reason for hiding this comment

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

@dengemann you need to update authors.rst too

@dengemann dengemann changed the title [ENH]: Add support for reading curry EEG files [MRG]: Add support for reading curry EEG files Sep 30, 2022
@agramfort
Copy link
Member

🙏 @dengemann

@agramfort agramfort merged commit de529c4 into mne-tools:main Oct 1, 2022
@welcome
Copy link

welcome bot commented Oct 1, 2022

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

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.

Add support for Curry .cdt input files
5 participants