-
Notifications
You must be signed in to change notification settings - Fork 90
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
Neuroscan Support #924
Neuroscan Support #924
Conversation
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
Codecov Report
@@ Coverage Diff @@
## main #924 +/- ##
==========================================
+ Coverage 94.71% 94.79% +0.07%
==========================================
Files 23 23
Lines 3499 3495 -4
==========================================
- Hits 3314 3313 -1
+ Misses 185 182 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
@yjmantilla can you add your name in this file
https://github.com/mne-tools/mne-bids/blob/main/doc/whats_new.rst
with a description of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaand could you add a small test please?
@agramfort Sure, im a bit busy but as soon as I can I will do that. @sappelhoff Yeah I could. Here is a public cnt dataset which may suffice: https://figshare.com/articles/dataset/The_original_EEG_data_for_driver_fatigue_detection/5202739 Should I add a single or a couple of this cnts file to mne.datasets.testing? Should I be following an example test? Im guessing it somehow has to do with mne-bids/mne_bids/tests/test_write.py Lines 118 to 123 in 4c0755c
Would it suffice adding the cnt to the testing datasets and append the CNT case to this list? |
maybe look at test functions like test_set or or test_bdf in test_write.py?
… |
In the
Cool, please add info about this PR under "enhancements" then. Note, that you also need to add your name to https://github.com/mne-tools/mne-bids/blob/main/doc/authors.rst in order for the link to your name to work.
agreed, or maybe this one: mne-bids/mne_bids/tests/test_write.py Line 1150 in 4c0755c
|
Hi @sappelhoff and @agramfort , sorry for the delay. I added the tests and updated what's new. For the tests I mainly imitated what I saw already implemented and made use of the variables I had to add some warning filters for cnt files inspired on:
found in this mne code I also had the problem of handedness set to None for the cnt file example. So I had to add also this warning filter: Maybe there is a better way to deal with that. make pep output:
make test output:
ci for docs-buikd is failing but maybe this is due to #941 , in correspondence my local build of the docs is also failing with the same error of the ci:
|
I just merged |
Im failing code coverage for some reason because of mne_bids/inspect.py although I didnt touch it. Otherwise it seems good mne_bids/config.py | 97.46% <ø> (ø) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @yjmantilla
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoechenberger merge if happy
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Thanks, @yjmantilla! |
PR Description
PR to add support of .cnt files.
Closes #922
Output of make pep just said:
I ran the tests locally and it said
237 passed
(at the end).Documentation build ended with:
Let me know if anything else is required, like an specific test for cnt files.
Merge checklist
Maintainer, please confirm the following before merging: