-
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
FIX: account for potential .{integer}_meg4
extensions in CTF format
#1230
Conversation
…ix .{int}_meg4 are also renamed
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
- Coverage 97.61% 97.59% -0.02%
==========================================
Files 40 40
Lines 8685 8663 -22
==========================================
- Hits 8478 8455 -23
- Misses 207 208 +1 ☔ View full report in Codecov by Sentry. |
Thanks for the PR @marakw! Could you please see my suggestion in https://mne.discourse.group/t/write-raw-bids-does-not-rename-all-data-files-of-the-ds-folder/8405/4?u=sappelhoff and consider that fix instead? something like: extra_ctf_file_types = [f".{i}_meg4" for i in range(1, 21)] # cap at 20 is arbitrary
file_types += extra_ctf_file_types I think that'd be cleaner |
Furthermore, please add an entry to "whats new" (the changelog) under "bug" here: https://github.com/mne-tools/mne-bids/blob/main/doc/whats_new.rst And please add yourself to the list of contributors here (right after Pierre, but before Alex): Line 170 in e41b517
|
.{integer}_meg4
extensions in CTF format
for more information, see https://pre-commit.ci
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.
great thanks! Do these changes fix the problem for you?
see also this documentation for more info about a proper dev setup: https://github.com/mne-tools/mne-bids/blob/main/CONTRIBUTING.md |
Thanks for your guidance @sappelhoff! Yeah the code works like this nicely on my dataset. I also pulled the changes again. Should I be concerned that some checks were not successful? |
let's see what happens when we let the entire CI run now :-) |
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 am happy with the PR as is, thanks for your contribution @marakw.
@hoechenberger merge if you're happy, please 🙏
Great, thank you. Happy to participate! Let me know if there is anything more I could do. |
@sappelhoff I won't have time to review soon; please go ahead and merge if you're happy :) |
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
In copyfile_ctf change suffix .meg4 to meg4 such that files with suffix .{int}_meg4 are also renamed.
PR Description
In copyfile_ctf change suffix .meg4 to meg4 such that files with suffix .{int}_meg4 are also renamed.
CTF datasets in .ds folders can be split into several files named .meg, .1_meg, .2_meg, ...
These files have until now been just copied from the original dataset without being renamed when using write_raw_bids.
The files are selected for renaming based on whether their name ends with the specified suffix, therefore this fix seems to solve the issue.
The problem was discussed here https://mne.discourse.group/t/write-raw-bids-does-not-rename-all-data-files-of-the-ds-folder/8405
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: