-
Notifications
You must be signed in to change notification settings - Fork 85
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
amended _write_electrodes_tsv
to exclude writing stim channels to electrodes.tsv
#1023
Conversation
…hey are not written to electrodes.tsv
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
Codecov Report
@@ Coverage Diff @@
## main #1023 +/- ##
=======================================
Coverage 95.17% 95.18%
=======================================
Files 25 25
Lines 3774 3777 +3
=======================================
+ Hits 3592 3595 +3
Misses 182 182
Help us with your feedback. Take ten seconds to tell us how you rate us. |
Great, can you add a test and a changelog entry please? |
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! +1 to merge once the changelog entry is there
Sure! I was going to add a test for this new behaviour using the 'test_egi.mff' in PR #1006 , but you're right it makes more sense to do it here. Will add the test and update changelog now. |
I don't see a I'm not sure if my test is too convoluted, I was trying to test for two cases:
|
mne_bids/tests/test_dig.py
Outdated
|
||
# test 1 | ||
with open(bids_path.directory / | ||
'sub-01_ses-01_acq-01_space-CapTrak_electrodes.tsv') as sidecar: |
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.
Should specify encoding='utf-8'
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.
done, thanks for the tip!
mne_bids/tests/test_dig.py
Outdated
# test 2 | ||
with pytest.warns(RuntimeWarning) as record: | ||
read_raw_bids(bids_path) | ||
# list of warnings emitted by read_raw_bids |
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.
I appreciate your writing this test (looks like a lot of work!), but I don't think we need this one … we just need to ensure we're writing the correct stuff (which the previous test ensures); testing the reading operation again is, IMHO, overkill. I'd suggest to remove test 2
.
@sappelhoff WDYT?
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.
Yeah, I was wondering if it was too much ha. I can remove it.
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
mne_bids/tests/test_dig.py
Outdated
@@ -334,7 +334,7 @@ def test_electrodes_io(tmp_path): | |||
raw.pick_types(eeg=True, stim=True) # we don't need meg channels | |||
bids_root = tmp_path / 'bids1' | |||
bids_path = _bids_path.copy().update(root=bids_root, datatype='eeg') | |||
os.makedirs(op.join(bids_root, 'sub-01', 'ses-01', 'eeg'), exist_ok=True) | |||
(bids_root / 'sub-01' / 'ses-01' / 'eeg').mkdir(exist_ok=True) |
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.
Why do you need to create this directory manually, anyway..?
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.
I'm not sure.. I was having a hard time understanding this, but was trying to follow the examples from the other tests in test.dig.py
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.
write_raw_bids
should create the directory tree, could you try without?
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
And yes, for the changelog it's in |
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.
@scott-huberty please also comment in #1028 with your full name, ORCID, and affiliation -- I'd like to add you there.
(@mscheltienne as well)
mne_bids/tests/test_dig.py
Outdated
assert n_entries == len( | ||
raw | ||
.copy() | ||
.pick_types(eeg=True) | ||
.ch_names | ||
) |
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.
seems my prior comment got lost: you should not need to do raw.copy()
here. use raw.get_channel_types()
and count the 'eeg'
s
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.
Nice trick, @drammock!
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.
awesome, thanks @drammock!
Co-authored-by: Mathieu Scheltienne <mathieu.scheltienne@gmail.com>
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.
I think those are the last points I see, + the encoding='utf-8'
to be added as mentioned by Richard.
I hope this is the last change we need to add support for the EGI format!
Co-authored-by: Mathieu Scheltienne <mathieu.scheltienne@gmail.com>
Co-authored-by: Mathieu Scheltienne <mathieu.scheltienne@gmail.com>
Thanks for that, @sappelhoff ! I think I commented in the right location of the zenodo json file.. Just Let me know if I commented in the wrong place. |
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Thanks @scott-huberty! |
Thanks everyone for the help! |
closes #1022
make tests
andmake pep
passed on my local machine, fingers crossed!