FIX: Remove redundant fmap_boldref resample in desc-fmapCoreg_bold.svg report#3640
Conversation
The 'fieldmap' input to init_func_fit_reports_wf is supplied by boldref_fmap (fit.py:500), which already reconstructs the fieldmap onto the BOLD reference grid. The fmap_boldref ApplyTransforms node added in niprepsgh-3387 was correct at the time but became redundant after niprepsgh-3467 rewired the input to come from boldref_fmap. The redundant resample applies boldref2fmap_xfm (inverted) to data already in BOLD space, shifting the fieldmap overlay in desc-fmapCoreg_bold.svg by ||boldref2fmap_xfm||. For acquisitions where BOLD and fieldmap have similar FOV anchors, the visible shift is within the brain and the figure looks fine; for acquisitions with substantially different FOV anchors (e.g. fieldmap reusing functional prescription with slice thickness/spacing decoupled), the overlay can render entirely outside the brain. The actual SDC correction is unaffected: unwarp_boldref (fit.py:614) and the time-series resample in apply.py both use ReconstructFieldmap outputs directly without going through fmap_boldref.
|
Thanks for the detailed analysis and PR! I've triggered the tests. I still need to digest these reports and the referenced PRs, but this is much very much appreciated. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3640 +/- ##
==========================================
- Coverage 74.57% 74.24% -0.33%
==========================================
Files 62 62
Lines 4975 4954 -21
Branches 637 532 -105
==========================================
- Hits 3710 3678 -32
- Misses 1131 1144 +13
+ Partials 134 132 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This is correct, but a bit disturbing. In general, we've tried to indicate the type and space of the thing being passed between workflows via the name, and it seems that I didn't follow that in #3467, which makes me worry that we aren't being consistent elsewhere.
I've updated the deeply outdated docstring so at least the space is documented here.
cc @mgxd as a heads up.
Closes #3639.
Changes proposed in this pull request
fmap_boldrefApplyTransformsnode frominit_func_fit_reports_wf.inputnode.fieldmapdirectly tosdcreg_report.fieldmap.This restores the wiring that existed before #3387, which is now correct because #3467 rewired
func_fit_reports_wf.inputnode.fieldmapto receiveboldref_fmap.out_file— a fieldmap already reconstructed onto the BOLD reference grid.Why
fmap_boldrefwas added in #3387 to resample a raw RAS fieldmap into BOLD space for theFieldmapReportlet. After #3467 introducedboldref_fmap(aReconstructFieldmapnode) and rewired its output into the reports workflow atfit.py:500, the resample became redundant. Removingfmap_boldrefrestores correct rendering. Actual SDC is unaffected —unwarp_boldref(fit.py:614) and the time-seriesresample(apply.py) useReconstructFieldmapoutputs directly. See the linked issue for figures and a centroid trace.Impact
desc-fmapCoreg_bold.svgnow plots the fieldmap at the correct location regardless of BOLD/fmap acquisition geometry.grep -rn fmap_boldrefreturns zero matches after the change.fmap_boldrefwas reports-only.Testing
init_func_fit_reports_wfundersdc_correction=True/False×freesurfer=True/Falseand walked the workflow graph:fmap_boldrefis gone, andsdcreg_report.fieldmapis connected directly frominputnode.fieldmappost-fix; the other three required inputs (reference,moving,mask) remain wired as before.25.2.5(figures in the linked issue) and verified the fix produces correctly-located overlays by invokingFieldmapReportletwith the proposed wiring.ruff check,ruff format --check, andcodespellpass on the modified file.Documentation that should be reviewed
None.