Skip to content

Conversation

@effigies
Copy link
Contributor

@effigies effigies commented May 22, 2025

The goal of this PR was initially to get SDC reports added to the ASL section of the reports when reusing minimal derivatives. The main thing that needed to happen was to reconstruct the fieldmap in the aslref space, which is part of the unwarp_wf workflow, deep inside the coregistration reference logic.

This PR applies nipreps/fmriprep@32478f8 (part of nipreps/fmriprep#3423) to split up the unwarp_wf into three nodes, and pulls the ReconstructFieldmap node into the new stage 3.

This needs applying to fMRIPrep, as well.

@effigies effigies force-pushed the fix/report-fieldmaps-full branch from 7f96d77 to 36aa373 Compare May 22, 2025 16:57
@tsalo tsalo added the refactor Changes to the codebase that don't impact workflow inputs or outputs. label May 22, 2025
@effigies effigies force-pushed the fix/report-fieldmaps-full branch from 0ac34b4 to 074820a Compare May 22, 2025 20:50
@effigies

This comment was marked as outdated.

@effigies effigies force-pushed the fix/report-fieldmaps-full branch from 074820a to 0084257 Compare May 22, 2025 20:51
Copy link
Contributor Author

@effigies effigies left a comment

Choose a reason for hiding this comment

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

A couple notes for reviewers.

'importlib_resources; python_version < "3.11"',
"acres",
"fmriprep >= 24.1.0, <= 25.0.0",
"fmriprep ~= 25.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to relax the requirements for a transform list in ResampleSeries, so it can just SD unwarp a single volume.

Comment on lines +40 to +42
"sdcflows ~= 2.13.0",
"sentry-sdk <= 2.28.0",
"smriprep <= 0.18.0",
"smriprep ~= 0.18.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ~= reflects that we try quite hard to maintain backwards compatibility during patch releases.

@tsalo tsalo requested review from Copilot, mattcieslak and tsalo May 23, 2025 14:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ASL fit workflow to isolate fieldmap reconstruction into its own stage and updates key dependency versions.

  • Updated version constraints for fmriprep, sdcflows, and smriprep in pyproject.toml
  • Split unwarp_wf logic into distinct stages: added Stage 3 for fieldmap reconstruction, Stage 4 for coregistration reference, and updated Stage 5 logging
  • Removed the old init_unwarp_wf import and reorganized workflow connections in fit.py

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pyproject.toml Bumped fmriprep, sdcflows, and smriprep to use ~= version syntax
aslprep/workflows/asl/fit.py Split ASL fit into separate unwarp, fieldmap, and coreg stages; updated node imports and connect calls
Comments suppressed due to low confidence (3)

aslprep/workflows/asl/fit.py:486

  • The ReconstructFieldmap node is used here but not imported; please add the appropriate import (e.g., from sdcflows.workflows.apply.fieldmap import ReconstructFieldmap) at the top of the file.
aslref_fmap = pe.Node(ReconstructFieldmap(inverse=[True]), name='aslref_fmap', mem_gb=1)

aslprep/workflows/asl/fit.py:553

  • [nitpick] The Stage 4 block duplicates much of the workflow connection logic used elsewhere; consider extracting shared setup steps into a helper function to reduce repetition and improve readability.
if not coreg_aslref:

aslprep/workflows/asl/fit.py:656

  • [nitpick] The log message for Stage 5 is ambiguous and similar to Stage 4; consider clarifying it (e.g., 'Stage 5: Adding ASL-to-anatomical registration workflow') to distinguish between stages.
config.loggers.workflow.info('Stage 5: Adding coregistration workflow')

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

This LGTM. Just want @mattcieslak to take a look as well before merging.

@tsalo tsalo merged commit ce0b3f6 into PennLINC:main May 23, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Changes to the codebase that don't impact workflow inputs or outputs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants