Skip to content

fix: Default to grid-constant interpolation mode #3516

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
Merged
merged 1 commit into from
Sep 17, 2025
Merged

fix: Default to grid-constant interpolation mode #3516

merged 1 commit into from
Sep 17, 2025

Conversation

@effigies
Copy link
Member

@effigies effigies commented Sep 4, 2025 •
edited
Loading

This is a potential fix to #3475 and #3508. To test:

git clone https://github.com/nipreps/fmriprep.git fmriprep-3516
git -C fmriprep-3516 fetch origin refs/pull/3516/head
git -C fmriprep-3516 checkout FETCH_HEAD
fmriprep-docker [NORMAL ARGS] --patch fmriprep=$PWD/fmriprep-3516/fmriprep

@claraElk @HippocampusGirl Could you verify that this does not re-introduce the problems we were trying to fix in #3453?

This was referenced Sep 4, 2025
Closed
Closed
@codecov
Copy link

codecov bot commented Sep 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.64%. Comparing base (cfe7a06) to head (bc5c33f).
⚠️ Report is 30 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3516   +/-   ##
=======================================
  Coverage   73.64%   73.64%           
=======================================
  Files          60       60           
  Lines        4759     4759           
  Branches      616      616           
=======================================
  Hits         3505     3505           
  Misses       1111     1111           
  Partials      143      143

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HippocampusGirl
Copy link
Contributor

Thanks for the heads-up! @claraElk and I are happy to have a look, but we likely need a week or two to re-run the analysis and get results

@effigies
Copy link
Member Author

effigies commented Sep 4, 2025

Sounds good. Will check in in two weeks.

HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this pull request Sep 9, 2025
@HippocampusGirl
Update to fmriprep development version for testing
e33a491 
See nipreps/fmriprep#3516
@effigies
Copy link
Member Author

@HippocampusGirl @claraElk Just checking in for results / an updated time estimate.

@effigies
Copy link
Member Author

effigies commented Sep 17, 2025 •
edited
Loading

For what it's worth, I reran my notebook from #3452:

image

I'm reasonably confident that this fixes the issue.

@HippocampusGirl
Copy link
Contributor

Hi @effigies, Thanks, that's great to see! Honestly, please feel free to merge already and we will let you know if we detect any issues on our end. I think we will need another few days to re-run everything, but @claraElk and I will definitely update you once we have results :-)

@effigies effigies requested a review from mgxd September 17, 2025 19:11
@mgxd mgxd merged commit 234cd68 into nipreps:master Sep 17, 2025
21 checks passed
@effigies effigies deleted the bp/grid-constant branch September 17, 2025 19:53
effigies added a commit that referenced this pull request Oct 1, 2025
@effigies
Merge tag '25.2.0'
13a2d44 
25.2.0 (October 01, 2025)

New feature release in the 25.2.x series.

This release is an *fMRIPrep Long-Term Support (LTS)* release.
The planned support window is 4 years, until October 2029.

This release is an incremental improvement over 25.1.x, but includes some fixes
and improvements that do not fit within our
`bug-fix policy <https://www.nipreps.org/devs/releases/#bug-fix-releases>`__.

Importantly, the change in interpolation in 25.1.0 introduced artifacts for some datasets.
This release changes the default interpolation mode to ``grid-constant``, which
resolves those problems while not reintroducing the issue the previous release sought to fix.

This release also (finally) introduces per-session processing.
The ``--session-label`` flag selects the sessions to process,
and the ``--subject-anatomical-reference`` flag indicates whether and how
to combine across sessions.
Existing filters passed via ``--bids-filter-file`` may need to be updated or
removed in favor of using these flags to achieve the desired behavior.

We would like to thank the AMP-SCZ and ENIGMA consortia for testing out and providing
feedback on this release.

  * FIX: Clean up output report language (#3529)
  * FIX: Default to grid-constant interpolation mode (#3516)
  * FIX: Adapt to transposed ndcoords in nitransforms (#3517)
  * FIX: Write out Freesurfer-derived outputs (#3512)
  * FIX: Add kwargs to _warnings.py (#3483)
  * ENH: Resample BOLD data to any surface template space using the Connectome Workbench (#3461)
  * ENH: Add boldref / sbref to source metadata (#3532)
  * ENH: Add dedicated session filtering, alternative anatomical template options (#3495)
  * ENH: Write out goodvoxels mask (#3513)
  * ENH: Add registration metadata to boldref-to-anat transforms (#3500)
  * ENH: Write out cortex mask GIFTIs (#3491)
  * ENH: Update transforms.py according to new transform chain of nitransforms (#3494)
  * RF/DOC: Improve and document command-line parser defaults (#3487)
  * DOC: Explain better SDC and B0FieldSource requirement (#2768)
  * DOC: Document `freesurfer` parameter in BOLD confound workflow init (#3504)
  * DOC: Add myself to contributor list (#3506)
  * DOC: Fix non-standard Input/Output docstring section management (#3505)
  * MNT: Split Dockerfile into base and pixi layers (#3521)
  * MNT: Replace conda with pixi and lock (#3503)
  * MNT: Update license metadata using SPDX expression (#3486)
  * MNT: no need to re-run `ruff check` after `ruff format` (#3480)
  * MNT: Update pre-commit ruff legacy alias (#3479)
@effigies effigies mentioned this pull request Oct 13, 2025
Closed
@claraElk
Copy link

Hi @effigies !

Sorry for the late reply. I extracted connectivity matrices using HALFpipe with fMRIPrep 20.2.7 and compared them with those obtained using HALFpipe with fMRIPrep 25.2.3, which includes the modifications mentioned above.

The results below show the average correlations across subjects for different motion confound parameters extracted by the two fMRIPrep versions (dataset ds000228, ~155 participants). I also examined the average correlations between the resulting connectivity matrices, which were above 0.9 (depending on the confound removal strategy).

Capture d’écran, le 2025-10-28 à 10 12 13

So, it looks like these changes did not reintroduce the problem, which is great news!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgxd mgxd Awaiting requested review from mgxd

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@effigies @HippocampusGirl @claraElk @mgxd