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

Add ability to delete files from a BIDS dataset #1149

Merged
merged 24 commits into from
Jun 30, 2023

Conversation

PierreGtch
Copy link
Contributor

@PierreGtch PierreGtch commented Jun 26, 2023

PR Description

Add a BIDSPath.rm method to delete all the matching files and update the scans.tsv and participants.tsv files accordingly

C.f. #547, #443

closes #439

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>"

@welcome
Copy link

welcome bot commented Jun 26, 2023

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

@sappelhoff sappelhoff requested a review from jasmainak June 26, 2023 09:22
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
@PierreGtch
Copy link
Contributor Author

I implemented the comments @agramfort made in PR #547.

@hoechenberger can take a look at the code now?

@PierreGtch PierreGtch marked this pull request as ready for review June 26, 2023 14:08
@PierreGtch PierreGtch changed the title Delete files [MRG] Delete files Jun 26, 2023
Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We finally have the entire CRUD operation stack!

mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
PierreGtch and others added 3 commits June 27, 2023 08:12
Co-authored-by: Mainak Jas <[email protected]>
Co-authored-by: Mainak Jas <[email protected]>
Co-authored-by: Mainak Jas <[email protected]>
@PierreGtch PierreGtch changed the title [MRG] Delete files [WIP] Delete files Jun 27, 2023
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, but please use list comprehensions instead of map()

mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/path.py Outdated Show resolved Hide resolved
mne_bids/tests/test_path.py Outdated Show resolved Hide resolved
mne_bids/tests/test_path.py Outdated Show resolved Hide resolved
@PierreGtch PierreGtch requested a review from hoechenberger June 27, 2023 09:19
@PierreGtch PierreGtch changed the title [WIP] Delete files Delete files Jun 28, 2023
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #1149 (7085a77) into main (eeb5903) will decrease coverage by 0.09%.
The diff coverage is 94.80%.

❗ Current head 7085a77 differs from pull request most recent head 2a14880. Consider uploading reports for the commit 2a14880 to get more accurate results

@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
- Coverage   97.59%   97.51%   -0.09%     
==========================================
  Files          40       40              
  Lines        8577     8654      +77     
==========================================
+ Hits         8371     8439      +68     
- Misses        206      215       +9     
Impacted Files Coverage Δ
mne_bids/path.py 97.38% <92.72%> (-0.33%) ⬇️
mne_bids/tests/test_path.py 99.85% <100.00%> (+<0.01%) ⬆️

... and 3 files with indirect coverage changes

@PierreGtch
Copy link
Contributor Author

@hoechenberger would you be happy with the current state?

@hoechenberger
Copy link
Member

I won't have time to review for the next few days, @sappelhoff can you take over please?

@sappelhoff
Copy link
Member

@PierreGtch could you please solve the remaining CI failure?

FileNotFoundError: fname does not exist: "/home/runner/work/mne-bids/mne-bids/MEG/sample/sample_audvis_trunc_raw.fif"

Other than that, this PR looks good to me.

@PierreGtch
Copy link
Contributor Author

@PierreGtch could you please solve the remaining CI failure?

FileNotFoundError: fname does not exist: "/home/runner/work/mne-bids/mne-bids/MEG/sample/sample_audvis_trunc_raw.fif"

@sappelhoff I actually found a very simple fix for this fixture, it should all be green now!

@hoechenberger hoechenberger changed the title Delete files Add ability to delete files from a BIDS dataset Jun 29, 2023
@sappelhoff
Copy link
Member

the bids validator issue (ubuntu-latest, 3.11, mne-main, validator-main test suite) can be ignored, however the failures on windows seem real to me 🤔

@PierreGtch
Copy link
Contributor Author

the bids validator issue (ubuntu-latest, 3.11, mne-main, validator-main test suite) can be ignored, however the failures on windows seem real to me 🤔

I managed to solve the windows issue 👌

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.

Thanks a lot!

@sappelhoff sappelhoff merged commit 9bc6fe5 into mne-tools:main Jun 30, 2023
@welcome
Copy link

welcome bot commented Jun 30, 2023

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

An updated function in the API for updating scans.tsv and participants files
4 participants