Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
==========================================
+ Coverage 84.17% 84.21% +0.04%
==========================================
Files 30 33 +3
Lines 2938 3104 +166
Branches 391 326 -65
==========================================
+ Hits 2473 2614 +141
- Misses 388 414 +26
+ Partials 77 76 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
57ccc47 to
21ab1c6
Compare
bcaf4ad to
9ecfbae
Compare
Three additions targeting the codecov/patch failure on nipreps#541: * sdcflows/interfaces/tests/test_warpkit.py (new) — instantiates every warpkit-backed interface so its spec class body is hit at import (was 0% on codecov despite running locally; likely an xdist worker-merge artefact). Also covers ``_as_str_list``, the ``_pkg`` invariant, and the ``border_filt=(1, 5)`` traits default regression. * sdcflows/workflows/tests/test_outputs.py (new) — direct construction test for ``init_fmap_derivatives_wf``, exercising both the default and ``write_dynamic=True`` paths. The MEDIC dynamic sinks branch was only reached transitively from the ``test_fmap_wf`` slow test, hence 0% patch coverage on outputs.py. * sdcflows/workflows/fit/tests/test_medic.py — fill the remaining helper gaps: empty-metadata rejection, ``sloppy=True`` zooms_min, ``_first``, and ``_temporal_mean`` for both 3D and 4D inputs. The ``_run_interface`` method bodies in interfaces/warpkit.py remain uncovered in the fast/slow envs since they require warpkit to import; the existing ``veryslow`` MEDIC fixtures exercise them when the optional dependency is installed.
|
I have no idea why test (3.13, pre, fast) is failing on the cache step. It looks like an out-of-space error on the runner (but the other jobs with the same steps succeeded, so very confused): |
|
@vanandrew I think someone must have rerun that failing job and now it's passing. |
tsalo
left a comment
There was a problem hiding this comment.
Thanks for this! I made a quick pass through the code.
|
Pushed four commits addressing your feedback (noise_frames, ctor inputs, nitransforms switch + interface cleanup, unified fmap output). |
|
I can't tell if these test failures are something wrong with my dataset additions or if it's just another one-off error: |
a4c4781 to
b070878
Compare
|
Looks like the CI keeps failing because it's running out of space (I added two datasets for tests so that might have pushed it over the edge). I pushed 6f5cfff2 adding a - name: Free disk space
uses: jlumbroso/free-disk-space@main
with:
tool-cache: false
android: true
dotnet: true
haskell: true
large-packages: false
swap-storage: falseThis gets rid of some unneeded development tools on the github runner to free room for data. |
|
@tsalo Alright. I made changes based on our convo today:
|
Three additions targeting the codecov/patch failure on nipreps#541: * sdcflows/interfaces/tests/test_warpkit.py (new) — instantiates every warpkit-backed interface so its spec class body is hit at import (was 0% on codecov despite running locally; likely an xdist worker-merge artefact). Also covers ``_as_str_list``, the ``_pkg`` invariant, and the ``border_filt=(1, 5)`` traits default regression. * sdcflows/workflows/tests/test_outputs.py (new) — direct construction test for ``init_fmap_derivatives_wf``, exercising both the default and ``write_dynamic=True`` paths. The MEDIC dynamic sinks branch was only reached transitively from the ``test_fmap_wf`` slow test, hence 0% patch coverage on outputs.py. * sdcflows/workflows/fit/tests/test_medic.py — fill the remaining helper gaps: empty-metadata rejection, ``sloppy=True`` zooms_min, ``_first``, and ``_temporal_mean`` for both 3D and 4D inputs. The ``_run_interface`` method bodies in interfaces/warpkit.py remain uncovered in the fast/slow envs since they require warpkit to import; the existing ``veryslow`` MEDIC fixtures exercise them when the optional dependency is installed.
da9d07b to
c55efd0
Compare
Three additions targeting the codecov/patch failure on nipreps#541: * sdcflows/interfaces/tests/test_warpkit.py (new) — instantiates every warpkit-backed interface so its spec class body is hit at import (was 0% on codecov despite running locally; likely an xdist worker-merge artefact). Also covers ``_as_str_list``, the ``_pkg`` invariant, and the ``border_filt=(1, 5)`` traits default regression. * sdcflows/workflows/tests/test_outputs.py (new) — direct construction test for ``init_fmap_derivatives_wf``, exercising both the default and ``write_dynamic=True`` paths. The MEDIC dynamic sinks branch was only reached transitively from the ``test_fmap_wf`` slow test, hence 0% patch coverage on outputs.py. * sdcflows/workflows/fit/tests/test_medic.py — fill the remaining helper gaps: empty-metadata rejection, ``sloppy=True`` zooms_min, ``_first``, and ``_temporal_mean`` for both 3D and 4D inputs. The ``_run_interface`` method bodies in interfaces/warpkit.py remain uncovered in the fast/slow envs since they require warpkit to import; the existing ``veryslow`` MEDIC fixtures exercise them when the optional dependency is installed.
c55efd0 to
b0a6904
Compare
|
@tsalo I did a rebase to the latest main, hopefully to quash the 3.13 test error that happened in the last CI run: https://github.com/nipreps/sdcflows/actions/runs/26792754492/job/79155816998#step:22:320 |
There was a problem hiding this comment.
There's a significant conceptual problem here. I'm not sure if it had been done correctly and then the distinction was lost after the latest refactor. If so, apologies for not getting to this sooner and saving you the time.
There is a comment that says the caller is responsible for ensuring that the fieldmap has already been projected into the moving (source) space, but we project them into the fixed (target) space.
To explain: We are resampling into the target space, so we need a VSM to correspond with every coordinate in the target space, and to then apply this shift in the source space. For a fixed fieldmap, this value is the same, so we can save computation by interpolating the fieldmap exactly once into the target space. For a dynamic fieldmap, there are no savings to be had here (and we don't know how to map the fieldmap into the target space without using the fieldmap to unwarp itself) and we need to interpolate the values in the target space. So if we want to have a single resampling function, it needs to have the logic:
if not dynamic:
vsm = fmap_hz * pe_info[1]
else:
vsm = ndi.map_coordinates(fmap_hz, coordinates, ...) * pe_info[1]
coordinates[pe_info[0], ...] += vsm
resampled = ndi.map_coordinates(data, coordinates)As it's a simple function, it might be cleaner to do two separate functions, one with a fieldmap in target space and one with it in source space. I don't know if that makes the wrapping code easier or harder.
I started making some other comments, but stopped when I got to this point. Happy to discuss further on this thread or on a call.
|
@effigies Thanks for the review! I'll take a closer look at the comments later this week. As for your conceptual concern, I think that comment might be a typo. I'm pretty sure I'm doing everything in the target space, not the moving space. But I'll take a look again and let you know for sure. |
@effigies Can you point out the line numbers of this comment? |
|
sdcflows/sdcflows/transform.py Lines 594 to 598 in a68a1e4 |
|
@effigies I took a closer look at this. It's more of a badly phrased docstring. I reworded it in 7b02d9a to make it clearer, but there are two separate concepts:
So we agree the apply path works in target space — the old docstring just described it as "moving/source," which is exactly the contradiction you flagged. MEDIC hands in a field that's already target-valued and on the EPI grid, so it skips |
Three additions targeting the codecov/patch failure on nipreps#541: * sdcflows/interfaces/tests/test_warpkit.py (new) — instantiates every warpkit-backed interface so its spec class body is hit at import (was 0% on codecov despite running locally; likely an xdist worker-merge artefact). Also covers ``_as_str_list``, the ``_pkg`` invariant, and the ``border_filt=(1, 5)`` traits default regression. * sdcflows/workflows/tests/test_outputs.py (new) — direct construction test for ``init_fmap_derivatives_wf``, exercising both the default and ``write_dynamic=True`` paths. The MEDIC dynamic sinks branch was only reached transitively from the ``test_fmap_wf`` slow test, hence 0% patch coverage on outputs.py. * sdcflows/workflows/fit/tests/test_medic.py — fill the remaining helper gaps: empty-metadata rejection, ``sloppy=True`` zooms_min, ``_first``, and ``_temporal_mean`` for both 3D and 4D inputs. The ``_run_interface`` method bodies in interfaces/warpkit.py remain uncovered in the fast/slow envs since they require warpkit to import; the existing ``veryslow`` MEDIC fixtures exercise them when the optional dependency is installed.
71f7ba7 to
aab352e
Compare
aab352e to
5614b98
Compare
|
I did a reorg of the git history for this since it was getting kind of intractable (~60 commits -> 8 commits). Also, rebased everything to the latest main branch. |
5614b98 to
bb04871
Compare
bb04871 to
8923a4e
Compare
|
Thanks. This is still on my to-do list. I think we're realistically going to need to focus on preserving old behavior and allow users to detect bugs in the new stuff, because I don't know that I have the bandwidth to review this as carefully as I would like. |
|
@effigies Alright. Then I'll work on re-implementing this as an optional separate pathway, while preserving the original code. |
|
@vanandrew Sorry, I didn't mean to imply that. I think keeping it as one pathway makes sense, and my review will just focus less on the correctness of the 4D branches and ensure the 3D branches are still doing what's expected. |
Generalize _resample_with_fieldmap so a per-volume (4D) fieldmap runs through the same code path as a static 3D one, selecting the matching frame for each volume. fieldmap_jacobian now takes the voxel shift map directly instead of recomputing it, so the static and dynamic callers share a single definition. Document the fmap_hz preconditions.
Add UnwrapPhase and ComputeFieldmap, wrapping warpkit's multi-echo phase unwrapping and fieldmap computation. Both follow the nipype num_threads convention. warpkit becomes a core dependency (>= 1.5.0). It carries a Washington University non-commercial license, so commercial use requires a separate WUSTL OTM agreement.
Add EstimatorType.MEDIC together with the is_dynamic property that tells per-volume fieldmaps apart from static ones. MEDIC estimation validates its file set up front, rejecting incomplete magnitude/phase part sets and single-echo input rather than failing deep inside the workflow.
Discover MEDIC estimators from B0FieldIdentifier and IntendedFor metadata only, never from directory structure alone. Cover the magnitude side of IntendedFor and de-duplicate estimators that both fields point at. Add --no-medic to opt out of MEDIC discovery entirely.
Fit workflow chaining UnwrapPhase into ComputeFieldmap to produce a per-volume fieldmap from complex multi-echo BOLD. fmap_ref comes from the raw first-echo magnitude and fmap_mask from warpkit's masks by way of init_dynamic_magnitude_wf, so dynamic estimators expose the same outputs as static ones.
Apply workflow that unwarps a BOLD series volume by volume against its per-volume fieldmap, with Jacobian intensity correction and the phase-encoding direction sign forwarded through to the fieldmap conversion. Resampling goes through nitransforms.
…rivatives Gate the static-versus-dynamic branch in init_fmap_preproc_wf on FieldmapEstimation.is_dynamic, and align the merges so non-MEDIC estimators keep working alongside MEDIC ones. The derivatives writer dismisses the task entity on dynamic ref/mask sinks and routes the dynamic magnitude reference through the fieldmap suffix.
Stage ds006926 and ds007637 (multi-echo magnitude + phase BOLD) in the cache-test-data job and run the MEDIC end-to-end tests on the veryslow lane. Free runner disk before the data cache is restored, since the added datasets push the root disk past its limit. Skip both datasets in conftest's layout auto-index -- indexing them stalls collection. Add Andrew Van to .zenodo.json.
8923a4e to
466802f
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
MEDIC has confirmed workflow-construction and reporting failures, and its dependency packaging contradicts the stated opt-in design.
Review effort: Balanced
Findings: 7
Open (13)
Forward no_medic in normal CLI execution · New Validate matching phase and magnitude echoes · New Skip coefficient outputs for dynamic estimators · New Validate pre-gridded map shape and affine · New Bound queued coordinate-grid task memory · New Seed outer MEDIC inputs from the estimator · New Provide matching 3D MEDIC report inputs · New Move warpkit to an opt-in extra · New Implement the missing force_medic discovery path · New Apply session filters to identifier file lookups · New Preserve IntendedFor MEDIC runs with other identifiers · New Pass omp_nthreads to warpkit interfaces · New Compute each echo's readout time with get_trt · New
What changed in this PR
This PR adds MEDIC fieldmap estimation and per-volume distortion correction to sdcflows for multi-echo magnitude and phase data.
Changes:
- Adds MEDIC detection, warpkit interfaces, and a fieldmap estimation workflow.
- Adds 4D fieldmap resampling and connects it to derivative and report workflows.
- Adds tests and CI fixtures; the current packaging makes warpkit a required dependency rather than the described opt-in extra.
| File | Description |
|---|---|
sdcflows/workflows/tests/test_outputs.py |
Checks 4D fieldmap merge configuration. |
sdcflows/workflows/outputs.py |
Allows 4D fieldmaps in derivative output. |
sdcflows/workflows/fit/tests/test_medic.py |
Tests MEDIC construction and metadata handling. |
sdcflows/workflows/fit/medic.py |
Builds the MEDIC estimation workflow. |
sdcflows/workflows/base.py |
Connects dynamic estimators to preprocessing outputs. |
sdcflows/workflows/apply/tests/test_dynamic.py |
Tests dynamic apply construction and resampling. |
sdcflows/workflows/apply/dynamic.py |
Adds the per-volume correction workflow. |
sdcflows/utils/wrangler.py |
Discovers and orders MEDIC estimators. |
sdcflows/utils/tests/test_wrangler.py |
Tests MEDIC discovery and opt-out behavior. |
sdcflows/transform.py |
Supports per-frame fieldmaps in resampling. |
sdcflows/tests/test_fieldmaps.py |
Tests MEDIC estimator validation. |
sdcflows/interfaces/warpkit.py |
Wraps warpkit’s estimation stages. |
sdcflows/interfaces/tests/test_warpkit.py |
Checks warpkit interface specifications. |
sdcflows/fieldmaps.py |
Adds the MEDIC estimator type and workflow dispatch. |
sdcflows/conftest.py |
Provides MEDIC test fixtures. |
sdcflows/config.py |
Adds MEDIC opt-out configuration. |
sdcflows/cli/parser.py |
Adds the --no-medic option. |
sdcflows/cli/main.py |
Passes the option through dry-run discovery. |
pyproject.toml |
Adds warpkit as a dependency. |
.zenodo.json |
Adds contributor metadata. |
.github/workflows/build-test-publish.yml |
Fetches MEDIC test data in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| layout=config.execution.layout, | ||
| subject=subject, | ||
| fmapless=config.workflow.fmapless, | ||
| no_medic=config.workflow.no_medic, |
| if len(phase_files) != len(mag_files): | ||
| raise ValueError( | ||
| f'MEDIC requires matched magnitude/phase pairs per echo; ' | ||
| f'got {len(phase_files)} phase and {len(mag_files)} ' | ||
| 'magnitude file(s).' | ||
| ) |
| elif self.method == EstimatorType.MEDIC: | ||
| from .workflows.fit.medic import init_medic_wf | ||
|
|
| fmap_img, _ = ensure_positive_cosines(self.mapped) | ||
| fmap_hz = np.asanyarray(fmap_img.dataobj, dtype='float32') |
| jacobian=jacobian, | ||
| num_threads=num_threads, |
| sessions: list[str] | None = None, | ||
| fmapless: bool | set = True, | ||
| force_fmapless: bool = False, | ||
| no_medic: bool = False, |
| ( | ||
| listify(ids) | ||
| for ids in layout.get_B0FieldIdentifiers(session=sessions, **base_entities) | ||
| ), |
| # MEDIC estimator). This block is the legacy fallback: when no | ||
| # ``B0FieldIdentifier`` is present, a complex multi-echo BOLD that declares | ||
| # ``IntendedFor`` is picked up here. Skipped entirely when ``no_medic`` is | ||
| # set or when ``B0FieldIdentifier`` metadata already drove discovery. | ||
| if not no_medic and not b0_ids: |
| unwrap = pe.Node( | ||
| UnwrapPhase(debug=debug), | ||
| name='unwrap', | ||
| n_procs=omp_nthreads, | ||
| ) | ||
|
|
||
| # ComputeFieldmap doesn't expose a ``debug`` input — only UnwrapPhase | ||
| # does, so the asymmetry is intentional. | ||
| compute_fmap = pe.Node( | ||
| ComputeFieldmap(), | ||
| name='compute_fmap', | ||
| n_procs=omp_nthreads, | ||
| ) |
| echo_times = [float(m['EchoTime']) * 1000.0 for m in metadata] | ||
| total_readout_time = float(metadata[0]['TotalReadoutTime']) | ||
| phase_encoding_direction = metadata[0]['PhaseEncodingDirection'] |


Summary
Adds multi-echo dynamic distortion correction (MEDIC) to sdcflows, backed by warpkit. MEDIC estimates a per-volume B0 fieldmap directly from a multi-echo, mag+phase BOLD series, capturing breathing- and motion-driven field changes that a single static fieldmap can't.
Revives #435 / #438 with the simpler pure-Python warpkit (post vanandrew/warpkit#16, no Julia/C++ setup required). Closes #36.
What's new
Workflows
init_medic_wf(sdcflows/workflows/fit/medic.py) — multi-echo phase + magnitude → 4D Hz fieldmap (one volume per timepoint, on the EPI grid) + brain-extracted reference + brain mask. Two-stage warpkit call (UnwrapPhase→ComputeFieldmap) so the per-frame masks stay accessible. Singlefmapoutput is 4D for MEDIC, leaving the 3D-vs-4D dispatch to the apply consumer.init_dynamic_unwarp_wf(sdcflows/workflows/apply/dynamic.py) — per-volume apply path built onsdcflows.transform.apply_dynamic_unwarp, a per-frame extension of the same scipy/nitransforms-backed resampling that powers the staticinit_unwarp_wf. No warpkit needed at runtime. Includes Jacobian determinant intensity correction (jacobian=Truedefault) sharingtransform.fieldmap_jacobianwith the static path.Interfaces
sdcflows/interfaces/warpkit.py— thinLibraryBaseInterfacewrappers around the two MEDIC stages SDCFlows actually drives:UnwrapPhase(ROMEO) andComputeFieldmap. Lazy import — sdcflows imports warpkit only when these interfaces actually run.Detection / dispatch
EstimatorType.MEDICadded.FieldmapEstimation.__attrs_post_init__detects MEDIC inputs (part-{phase,mag}onbold/epi/sbrefsources), enforces matched echo cardinality (≥2 phase, equal mag count), and rejects partial or mixed part sets.get_workflowinstantiatesinit_medic_wfwithEchoTime-sorted lists (BIDS doesn't guaranteeechoentity == numeric order).part='phase'andpart='mag'queries (some datasets carryIntendedForonly on one side); dedup walks the full sibling set, no reliance on pybids ordering.force_medicopt-in onfind_estimators— auto-discover MEDIC estimators from complex multi-echo BOLD even when neitherIntendedFornorB0FieldIdentifieris set. Pairing is unambiguous because the part-mag/part-phase echoes of the same run are MEDIC sources by construction. Intended for public datasets that ship the required echoes without the metadata the default discovery path needs.Plumbing
init_fmap_preproc_wfskipsfmap_coefffor MEDIC (the fieldmap is on the EPI grid by construction, no B-spline rep).init_fmap_derivatives_wfusesMergeSeries(allow_4D=True)so MEDIC's 4Dfmappasses through the sameds_fieldmapsink as the 3D static fmaps.Packaging
warpkitextra inpyproject.toml, explicitly excluded from[all]because warpkit ships under a non-commercial WUSTL license. Defaultpip install sdcflowsstays Apache-clean.tox.ini: theveryslowenv pulls the warpkit extra so MEDIC end-to-end tests only run there.v3).Validation
echo=Query.REQUIRED),FieldmapEstimationcardinality check (len(phase_files) < 2), and_unpack_metadataruntime guard insideinit_medic_wf.test_apply_dynamic_unwarp_matches_staticpinsapply_dynamic_unwarpto the same Hz→VSM + scipy.ndimage convention as the static_sdc_unwarppath — catches drift in sign / pe_info handling.test_wrangler_filter/test_wrangler_URIsparametrized with a 3-session × 3-echo × {mag,phase} BIDS skeleton.Known compromises
IntendedFor/B0FieldIdentifier— same constraint as the existing single-PE EPI branch. Datasets missing both can opt into discovery viaforce_medic=Trueonfind_estimators(see Detection / dispatch above).fmapoutput is 4D for MEDIC. Downstream tools that expect 3D field maps need to either dispatch on dimensionality or block MEDIC-based estimators until they do.Test plan
pytest sdcflows/utils/tests/test_wrangler.py— wrangler MEDIC detection paths (includingforce_medic)pytest sdcflows/workflows/fit/tests/test_medic.py—init_medic_wfconstruction +_unpack_metadataguardspytest sdcflows/workflows/apply/tests/test_dynamic.py—init_dynamic_unwarp_wfconstruction + jacobian flag + per-frame resampling vs. static pathpytest -m veryslowwithpip install sdcflows[warpkit]and ds006926 / ds007637 fixtures present — full MEDIC fit + dynamic apply