FIX: Refactoring the `B0FieldTransform` implementation #346

oesteban · 2023-04-07T10:51:06Z

This PR defines a new interface for the ApplyCoeffsField node, and dismisses TransformCoefficients because that projection is really not necessary.

Resolves: #345.

sdcflows/transform.py

oesteban · 2023-04-07T13:34:06Z

@effigies thanks much for looking at this draft -- this is a very good moment to give me feedback since I don't plan to work on this for a few hours.

effigies · 2023-04-07T19:17:33Z

@oesteban Everything here so far makes sense. I do think the idea of returning to Hz and then resampling that scalar map seems almost guaranteed to be fine. We could do a 2-4x upsampling when generating the $\mathbf{F}$-space fieldmap. Even a linear interpolation with the transform into $\mathbf{B}$ space would probably be a good enough approximation, then.

oesteban · 2023-06-30T11:27:28Z

@effigies @mgxd I'm finally unlocking this. I have made great progress this morning - it'd be nice to get a fast double-check of my latest commit 50a8e64.

As the TODO comment there anticipates, I'm now working toward integrating head motion correction within the interpolation framework.

codecov-commenter · 2023-06-30T11:31:30Z

Codecov Report

Patch coverage: 77.16% and project coverage change: -1.46% ⚠️

Comparison is base (375dd7a) 81.99% compared to head (6e3de3a) 80.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #346      +/-   ##
==========================================
- Coverage   81.99%   80.53%   -1.46%     
==========================================
  Files          26       26              
  Lines        2271     2296      +25     
  Branches      281      286       +5     
==========================================
- Hits         1862     1849      -13     
- Misses        362      402      +40     
+ Partials       47       45       -2

Flag	Coverage Δ
unittests	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
sdcflows/workflows/apply/correction.py	`82.75% <ø> (-0.58%)`	⬇️
sdcflows/workflows/fit/syn.py	`48.50% <ø> (ø)`
sdcflows/workflows/apply/registration.py	`95.00% <66.66%> (+6.53%)`	⬆️
sdcflows/transform.py	`77.29% <73.19%> (-12.85%)`	⬇️
sdcflows/interfaces/bspline.py	`68.92% <92.00%> (-5.70%)`	⬇️
sdcflows/workflows/fit/pepolar.py	`100.00% <100.00%> (ø)`

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

effigies · 2023-06-30T11:46:53Z

Mathias is off. I'll review in an hour or so.

oesteban · 2023-06-30T12:00:08Z

No rush, if not possible it's fine too - I'll keep going and we can have that look at the end. Thanks!

sdcflows/interfaces/bspline.py

oesteban · 2023-06-30T16:16:21Z

The last commit should be a candidate to give this an end... See the git commit message for more details. 🚀

effigies

Couple big issues. Still looking through this.

sdcflows/transform.py

effigies

Okay, I've read through this now. Only one additional comment, and it's more of a nit-pick as series with degenerate spatial dimensions are probably going to break in other ways...

sdcflows/transform.py

oesteban · 2023-07-03T15:43:44Z

Okay, I've been working on the docstring and on pacifying tests.

I have opened #369 for a test that will require further investigation.

This could be closed after beefing up tests a little. This is the plan:

1 - Add a recent EPI "pepolar" correction pair, and one fmap2epi affine. I have data I can distribute for this, with DWI with 6 b=0 acquired sufficient time apart to have visible head motion.
2 - Realign these b=0 and add them to the test data so that we can check the simultaneous hmc and sdc.

WDYT? @effigies @mgxd

oesteban · 2023-07-03T15:47:25Z

BTW, these EPI are LAS I have them for both LR/RL and PA/AP PEs.

mgxd

A few passing comments

sdcflows/interfaces/bspline.py

sdcflows/transform.py

oesteban · 2023-08-11T15:15:50Z

@oesteban Those outputs look correct to me.

Just thinking about naming schemes (as we don't have fieldmap derivatives in any BEP), here's a thought for a naming scheme, preserving the order you have:
* `fmap/sub-101006_coeff-1_desc-<B0FieldIdentifier>_fieldmap.nii.gz`

* `fmap/sub-101006_desc-<B0FieldIdentifier>_fmapref.nii.gz`

* `fmap/sub-101006_desc-<B0FieldIdentifier>_mask.nii.gz`

* `func/sub-101006_task-rest_dir-LR_from-boldref_to-<B0FieldIdentifier>_mode-image_xfm.mat`

* `func/sub-101006_task-rest_dir-LR_desc-preproc_boldref.nii.gz`

* `func/sub-101006_task-rest_dir-LR_desc-brain_mask.nii.gz`
The idea is to try to be abstracted from the details of whether our boldref is derived from bold or sbref, and what type of fieldmap we use to generate fieldmap and fmapref. It's possible that we do want to specifically use different fmaprefs depending on the field source, as epi and magnitude suffixes will indicate a need for different registration parameters.

Can we maybe go ahead without tinkering with the test dataset and keep this in mind when we write the CLI of SDCFlows? I think we are not far from pinning that one down and your thoughts are very useful there.

oesteban · 2023-08-11T15:25:57Z

Okay, believe it or not, this is coming to an end :)

I have decided to leave the head motion correction transforms out of this PR (see ~~1e5dbe4~~, force-pushed 0f24b6b) because testing that will require some extra effort and we may want to give priority to the jacobians instead.

Should we try to merge this today?

I've checked and all tests are looking gorgeous (I'm even surprised how the phasediff correction is on pair with the quality level of pepolar maps).

I leave it in your hands @effigies and @mgxd .

effigies · 2023-08-11T15:40:49Z

Can we maybe go ahead without tinkering with the test dataset and keep this in mind when we write the CLI of SDCFlows? I think we are not far from pinning that one down and your thoughts are very useful there.

Agreed. Absolutely does not need doing here.

oesteban · 2023-08-15T10:20:56Z

Thumbs up to merging? @mgxd @effigies

effigies · 2023-08-15T11:10:20Z

Sorry, I started a final review on Friday and wasn't able to get back to it yesterday. Will do it first thing today.

mgxd

gave another quick pass

mgxd · 2023-08-09T14:57:24Z

sdcflows/workflows/apply/correction.py

-                                    ("out_warp", "fieldwarp_ref")]),
+        (resample, outputnode, [("out_field", "fieldmap")]),
+        # (resample_ref, outputnode, [("out_field", "fieldmap_ref")]),
+        # TODO - take reference from brainextraction workflow


still todo?

sdcflows/interfaces/bspline.py

sdcflows/interfaces/tests/test_bspline.py

sdcflows/transform.py

mgxd · 2023-08-15T14:18:26Z

sdcflows/transform.py

+            target_reference = deoblique_and_zooms(
+                listify(self.coeffs)[-1],
+                target_reference,
+            )

        # Generate tensor-product B-Spline weights
        for level in listify(self.coeffs):


listify is potentially called 3 times on self.coeffs - perhaps we save the cycles and just set to a variable?

sdcflows/transform.py

sdcflows/workflows/apply/registration.py

effigies

Okay. Overall looks good. Most suggestions are for maintainability, not bugs caught.

sdcflows/transform.py

sdcflows/interfaces/bspline.py

sdcflows/transform.py

sdcflows/workflows/apply/correction.py

sdcflows/transform.py

sdcflows/interfaces/bspline.py

sdcflows/workflows/tests/test_integration.py

sdcflows/transform.py

Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu> Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>

oesteban · 2023-08-16T13:08:35Z

@effigies it looks our tests dislike the new version of pybids released two days ago:

ImportError while loading conftest '/home/runner/work/sdcflows/sdcflows/sdcflows/conftest.py'.
sdcflows/conftest.py:39: in <module>
    layouts = {
sdcflows/conftest.py:40: in <dictcomp>
    p.name: BIDSLayout(str(p), validate=False, derivatives=True)
/usr/share/miniconda/envs/test/lib/python3.10/site-packages/bids/layout/layout.py:177: in __init__
    _indexer(self)
/usr/share/miniconda/envs/test/lib/python3.10/site-packages/bids/layout/index.py:154: in __call__
    self._index_metadata()
/usr/share/miniconda/envs/test/lib/python3.10/site-packages/bids/layout/index.py:484: in _index_metadata
    tag = _create_tag_dict(bf, all_entities[md_key], md_val, is_metadata=True)
/usr/share/miniconda/envs/test/lib/python3.10/site-packages/bids/layout/models.py:720: in _create_tag_dict
    raise ValueError(
E   ValueError: Passed value has an invalid dtype (NoneType). Must be one of int, float, bool, or str.

Do you have any quick ideas of why this is happening?

EDIT: seems related to bids-standard/pybids#1009

setup.cfg

effigies · 2023-08-16T13:43:11Z

According to git bisect, bids-standard/pybids#1013 is where this started to fail.

Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu>

effigies · 2023-08-16T20:16:57Z

🎉

…3077) ## Changes proposed in this pull request * Accommodate nipreps/sdcflows#346 * Convert ITK transforms from `.mat` (MATLAB) to text format

effigies reviewed Apr 7, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

oesteban mentioned this pull request Apr 27, 2023

ENH: Function to calculate reference grids aligned with the coefficients #355

Merged

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch 4 times, most recently from ce25d24 to 5617345 Compare May 2, 2023 12:59

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 75be890 to fe4faf1 Compare June 30, 2023 07:48

effigies reviewed Jun 30, 2023

View reviewed changes

sdcflows/interfaces/bspline.py Show resolved Hide resolved

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 19513f6 to dea2408 Compare June 30, 2023 16:15

oesteban marked this pull request as ready for review June 30, 2023 16:16

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch 2 times, most recently from 182f3e5 to 001c3f9 Compare June 30, 2023 19:15

effigies reviewed Jun 30, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

sdcflows/transform.py Outdated Show resolved Hide resolved

sdcflows/transform.py Outdated Show resolved Hide resolved

effigies reviewed Jun 30, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

oesteban commented Jul 1, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch 4 times, most recently from 6a8c5c3 to 6678591 Compare July 3, 2023 11:32

oesteban mentioned this pull request Jul 3, 2023

Revise tolerance of sdcflows/interfaces/tests/test_bspline.py::test_bsplines #369

Open

mgxd reviewed Jul 5, 2023

View reviewed changes

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from da4c183 to 3266cf8 Compare August 11, 2023 14:05

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 3266cf8 to 1e5dbe4 Compare August 11, 2023 15:22

enh: disable head-motion transforms consideration

0f24b6b

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 1e5dbe4 to 0f24b6b Compare August 11, 2023 15:27

mgxd reviewed Aug 15, 2023

View reviewed changes

effigies reviewed Aug 15, 2023

View reviewed changes

oesteban commented Aug 16, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

oesteban commented Aug 16, 2023

View reviewed changes

sdcflows/transform.py Outdated Show resolved Hide resolved

oesteban and others added 2 commits August 16, 2023 10:41

Apply suggestions from code review

2e72608

Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu> Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>

fix+sty: argument specification + run black

107f56e

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from c78c74e to 107f56e Compare August 16, 2023 08:57

effigies reviewed Aug 16, 2023

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 515d8c9 to 3c024ea Compare August 16, 2023 13:23

Apply suggestions from code review

ea12e7f

Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu>

oesteban force-pushed the fix/yoio-you-only-interpolate-once branch from 779c239 to ea12e7f Compare August 16, 2023 14:14

enh: address comments from code review

6e3de3a

Co-authored-by: Mathias Goncalves <mathiasg@stanford.edu>

effigies mentioned this pull request Aug 16, 2023

ENH: Allow unwarp to apply only to references #385

Closed

oesteban merged commit 11fadb2 into master Aug 16, 2023
16 of 18 checks passed

oesteban deleted the fix/yoio-you-only-interpolate-once branch August 16, 2023 20:14

This was referenced Aug 21, 2023

RF: Update workflow to use sdcflows@master unwarp_wf nipreps/fmriprep#3075

Closed

FIX: Update connections to unwarp_wf, convert ITK transforms to text nipreps/fmriprep#3077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Refactoring the `B0FieldTransform` implementation #346

FIX: Refactoring the `B0FieldTransform` implementation #346

oesteban commented Apr 7, 2023

oesteban commented Apr 7, 2023

effigies commented Apr 7, 2023

oesteban commented Jun 30, 2023

codecov-commenter commented Jun 30, 2023 •

edited by codecov bot

Loading

effigies commented Jun 30, 2023

oesteban commented Jun 30, 2023

oesteban commented Jun 30, 2023

effigies left a comment

effigies left a comment

oesteban commented Jul 3, 2023 •

edited

Loading

oesteban commented Jul 3, 2023

mgxd left a comment

oesteban commented Aug 11, 2023

oesteban commented Aug 11, 2023 •

edited

Loading

effigies commented Aug 11, 2023

oesteban commented Aug 15, 2023

effigies commented Aug 15, 2023 •

edited

Loading

mgxd left a comment

mgxd Aug 9, 2023

mgxd Aug 15, 2023

effigies left a comment

oesteban commented Aug 16, 2023 •

edited

Loading

effigies commented Aug 16, 2023

effigies commented Aug 16, 2023

FIX: Refactoring the B0FieldTransform implementation #346

FIX: Refactoring the B0FieldTransform implementation #346

Conversation

oesteban commented Apr 7, 2023

oesteban commented Apr 7, 2023

effigies commented Apr 7, 2023

oesteban commented Jun 30, 2023

codecov-commenter commented Jun 30, 2023 • edited by codecov bot Loading

Codecov Report

effigies commented Jun 30, 2023

oesteban commented Jun 30, 2023

oesteban commented Jun 30, 2023

effigies left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban commented Jul 3, 2023 • edited Loading

oesteban commented Jul 3, 2023

mgxd left a comment

Choose a reason for hiding this comment

oesteban commented Aug 11, 2023

oesteban commented Aug 11, 2023 • edited Loading

effigies commented Aug 11, 2023

oesteban commented Aug 15, 2023

effigies commented Aug 15, 2023 • edited Loading

mgxd left a comment

Choose a reason for hiding this comment

mgxd Aug 9, 2023

Choose a reason for hiding this comment

mgxd Aug 15, 2023

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban commented Aug 16, 2023 • edited Loading

effigies commented Aug 16, 2023

effigies commented Aug 16, 2023

FIX: Refactoring the `B0FieldTransform` implementation #346

FIX: Refactoring the `B0FieldTransform` implementation #346

codecov-commenter commented Jun 30, 2023 •

edited by codecov bot

Loading

oesteban commented Jul 3, 2023 •

edited

Loading

oesteban commented Aug 11, 2023 •

edited

Loading

effigies commented Aug 15, 2023 •

edited

Loading

oesteban commented Aug 16, 2023 •

edited

Loading