mne-tools / mne-bids

MNE-BIDS is a Python package that allows you to read and write BIDS-compatible datasets with the help of MNE-Python.
https://mne.tools/mne-bids/
BSD 3-Clause "New" or "Revised" License
126 stars 84 forks source link

FIX: account for potential `.{integer}_meg4` extensions in CTF format #1230

Closed marakw closed 4 months ago

marakw commented 4 months ago

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, .2meg, ... 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:

welcome[bot] commented 4 months ago

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

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.59%. Comparing base (87eea28) to head (2b6603e). Report is 7 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sappelhoff commented 4 months ago

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

sappelhoff commented 4 months ago

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): https://github.com/mne-tools/mne-bids/blob/e41b5173a8c18ab60b620861d6ba8743bd2bec0f/CITATION.cff#L170

sappelhoff commented 4 months ago

Please note that you will have to git pull before continuing to work on this, as our CI has made a change on your remote branch:

image

sappelhoff commented 4 months ago

see also this documentation for more info about a proper dev setup: https://github.com/mne-tools/mne-bids/blob/main/CONTRIBUTING.md

marakw commented 4 months ago

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?

sappelhoff commented 4 months ago

Should I be concerned that some checks were not successful?

let's see what happens when we let the entire CI run now :-)

marakw commented 4 months ago

Great, thank you. Happy to participate! Let me know if there is anything more I could do.

hoechenberger commented 4 months ago

@sappelhoff I won't have time to review soon; please go ahead and merge if you're happy :)

welcome[bot] commented 4 months ago

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