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.

Sign up for GitHub

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

Jump to bottom

[FIX] Allow preprocessed data in native space in load_confounds #3531

Merged
merged 9 commits into from
Mar 3, 2023
Merged

[FIX] Allow preprocessed data in native space in load_confounds #3531

merged 9 commits into from
Mar 3, 2023

Conversation

htwangtw
Copy link
Member

Closes #3479.

Changes proposed in this pull request:

  • Use nilearn.interfaces.bids.parse_bids_filename to parse and construct file names that fits fMRIPrep doc better
  • Adapt BIDS compatible test file name

@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2023 •
edited by htwangtw
Loading

👋 @htwangtw Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

  • PR has an interpretable title.
  • PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
  • Code is PEP8-compliant (see our documentation on coding style)
  • Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)
    For new features:
  • There is at least one unit test per new function / class (see our documentation on testing)
  • The new feature is demoed in at least one relevant example.
    For bug fixes:
  • There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

@codecov
Copy link

codecov bot commented Feb 22, 2023 •
edited
Loading

Codecov Report

Merging #3531 (24436a1) into main (bf2eb51) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3531   +/-   ##
=======================================
  Coverage   91.01%   91.01%           
=======================================
  Files         133      133           
  Lines       15384    15392    +8     
  Branches     3212     3214    +2     
=======================================
+ Hits        14001    14009    +8     
  Misses        819      819           
  Partials      564      564
Impacted Files Coverage Δ
nilearn/interfaces/fmriprep/tests/utils.py 100.00% <ø> (ø)
...ilearn/interfaces/fmriprep/load_confounds_utils.py 100.00% <100.00%> (ø)
...n/interfaces/fmriprep/tests/test_load_confounds.py 98.34% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Remi-Gau
Remi-Gau reviewed Feb 23, 2023
View reviewed changes
nilearn/interfaces/fmriprep/load_confounds_utils.py Outdated
suffix = "_space-" + nii_file.split("space-")[1]
entities = parse_bids_filename(nii_file)
subject_label = f"sub-{entities['sub']}"
if "session" in entities:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still wrapping my head around how nilearn deals with BIDS things but isn't that supposed to be

if "ses" in entities:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and the test is missing this line. Will fix it

bthirion
bthirion reviewed Feb 28, 2023
View reviewed changes
nilearn/interfaces/fmriprep/load_confounds_utils.py Outdated
"nii.gz": "_space-.*_desc-preproc_bold.nii.gz",
"dtseries.nii": "_space-.*_bold.dtseries.nii",
"func.gii": "_space-.*_hemi-[LR]_bold.func.gii",
"nii.gz": "(_space-.*)?_desc-preproc_bold\.nii\.gz",
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: what is the effects of \ in the string ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it escapes the . as a wildcard character in regular expressions

bthirion
bthirion approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

LGTM, thx.

ymzayek
ymzayek reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@ymzayek ymzayek left a comment

Choose a reason for hiding this comment

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

LGTM @htwangtw! I think this is worth a whatsnew entry

nilearn/interfaces/fmriprep/load_confounds_utils.py Outdated Show resolved Hide resolved
@ymzayek
Copy link
Member

ymzayek commented Mar 2, 2023

@htwangtw since I made a prerelease earlier this week the latest.rst file changed. Can you rebase on main and move the whatsnew entry to the new section?

@htwangtw htwangtw force-pushed the load-confounds-native-space branch from 97b7c4d to 24436a1 Compare March 2, 2023 15:52
ymzayek
ymzayek approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@ymzayek ymzayek left a comment

Choose a reason for hiding this comment

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

LGTM thx @htwangtw !

@ymzayek ymzayek merged commit 9a84e7d into nilearn:main Mar 3, 2023
@htwangtw htwangtw deleted the load-confounds-native-space branch March 3, 2023 16:42
michellewang pushed a commit to michellewang/nilearn that referenced this pull request Mar 8, 2023
@htwangtw @ymzayek @michellewang
[FIX] Allow preprocessed data in native space in load_confounds (ni…
3a854f3 
…learn#3531)

* [ADD/BREAK]Use BIDS entity to look up confound file

* FIX correctly detect native space

* [TEST]session and run in file name

* Update nilearn/interfaces/fmriprep/load_confounds_utils.py

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

* Update latest

* FIX extra symbols?

* typo...

* remove \. escape to avoid deprecation warning

* move update to dev section

---------

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Remi-Gau Remi-Gau Remi-Gau left review comments

@bthirion bthirion bthirion approved these changes

@ymzayek ymzayek ymzayek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Less restrictive of load_confounds file mathcing pattern
4 participants
@htwangtw @ymzayek @bthirion @Remi-Gau