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

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

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

marakw
Copy link
Contributor

@marakw marakw commented Feb 29, 2024

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:

  • 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
  • PR description includes phrase "closes <#issue-number>"

Copy link

welcome bot commented Feb 29, 2024

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

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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.
📢 Have feedback on the report? Share it here.

@sappelhoff
Copy link
Member

sappelhoff commented Feb 29, 2024

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
Copy link
Member

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):

- given-names: Pierre

@sappelhoff sappelhoff added this to the 0.15 milestone Feb 29, 2024
@sappelhoff sappelhoff changed the title In copyfile_ctf change suffix .meg4 to meg4 such that files with suff… FIX: account for potential .{integer}_meg4 extensions in CTF format Feb 29, 2024
Copy link
Member

@sappelhoff sappelhoff left a 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?

doc/whats_new.rst Show resolved Hide resolved
@sappelhoff
Copy link
Member

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
Copy link
Member

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

@marakw
Copy link
Contributor Author

marakw commented Feb 29, 2024

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
Copy link
Member

Should I be concerned that some checks were not successful?

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

Copy link
Member

@sappelhoff sappelhoff left a 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 🙏

@marakw
Copy link
Contributor Author

marakw commented Feb 29, 2024

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

@hoechenberger
Copy link
Member

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

@sappelhoff sappelhoff merged commit 28e8cdb into mne-tools:main Feb 29, 2024
18 of 19 checks passed
Copy link

welcome bot commented Feb 29, 2024

🎉 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.

3 participants