Skip to content

Conversation

@mgxd
Copy link
Contributor

@mgxd mgxd commented Jul 7, 2021

Because of the configuration we're using, TOPUP fails when given an image with an odd number of slices. This adds a new interface PadSlice that adds an additional empty slice to images with odd numbered slices.

Once I test this, I'll convert out of draft mode.

A few questions:

  • Does the pad interface need to take into account image orientation?
  • Do we want to remove the added slice after calculating TOPUP correction?

@mgxd
Copy link
Contributor Author

mgxd commented Jul 8, 2021

@oesteban I'm a bit confused on how the best way to handle this added slice further down the pipeline.

TOPUP now proceeds without error, but

fix_coeff = pe.Node(
TOPUPCoeffReorient(), name="fix_coeff", run_without_submitting=True
)

complains of a shape mismatch between the regridded reference fieldmap and the coefficients file.

Would it be better to:

  • Assign the padded image as the reference? or
  • Remove a slice from the coeffs generated by TOPUP?

@oesteban
Copy link
Member

oesteban commented Jul 8, 2021

complains of a shape mismatch between the regridded reference fieldmap and the coefficients file.

where exactly? I would expect this, but I don't know where precisely. My first intuition would be to use the padded image as reference for the coefficients.

@mgxd
Copy link
Contributor Author

mgxd commented Jul 8, 2021

This is the check

coeff_shape = np.array(coeffnii.shape[:3])
ref_shape = np.array(refnii.shape[:3])
factors = coeffnii.header.get_zooms()[:3]
if not np.all(coeff_shape == ref_shape // factors + 3):
raise ValueError(
f"Shape of coefficients file {coeff_shape} does not meet the "
f"expectation given the reference's shape {ref_shape}."
)

For now, I'm testing padding the reference image as well.

@oesteban
Copy link
Member

oesteban commented Jul 8, 2021

Yes, use the padded reference for this interface too 👍

@mgxd mgxd marked this pull request as ready for review July 9, 2021 16:46
@mgxd mgxd requested a review from oesteban July 9, 2021 16:46
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Left a few comments.

@pep8speaks
Copy link

pep8speaks commented Aug 5, 2021

Hello @mgxd, Thank you for updating!

Line 138:5: E303 too many blank lines (2)

To test for issues locally, pip install flake8 and then run flake8 sdcflows.

Comment last updated at 2021-08-24 16:54:14 UTC

@mgxd
Copy link
Contributor Author

mgxd commented Aug 5, 2021

I think this is ready, I've opened up #220 to note the unrelated circle failure

@effigies
Copy link
Member

effigies commented Aug 5, 2021

I would want to use a few orientations to test that the same ras+ locations have the same values, to make sure we're correctly padding with respect to the affine.

@mgxd
Copy link
Contributor Author

mgxd commented Aug 24, 2021

forgot to follow up here, sorry for the delay - @effigies can you take a look at fdb9dbc and lmk what's needed to push this along?

@codecov-commenter
Copy link

Codecov Report

Merging #217 (7d25bc9) into master (e6d33e6) will increase coverage by 3.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   93.56%   97.13%   +3.57%     
==========================================
  Files          24       24              
  Lines        1647     1676      +29     
  Branches      191      192       +1     
==========================================
+ Hits         1541     1628      +87     
+ Misses         81       25      -56     
+ Partials       25       23       -2     
Flag Coverage Δ
travis 88.96% <100.00%> (+0.19%) ⬆️
unittests 97.12% <100.00%> (+3.70%) ⬆️

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

Impacted Files Coverage Δ
sdcflows/interfaces/utils.py 98.50% <100.00%> (+17.57%) ⬆️
sdcflows/workflows/fit/pepolar.py 100.00% <100.00%> (ø)
sdcflows/interfaces/brainmask.py 100.00% <0.00%> (+43.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d33e6...7d25bc9. Read the comment docs.

@mgxd mgxd merged commit 73eb0d0 into nipreps:master Aug 27, 2021
@mgxd mgxd deleted the enh/pad-topup branch August 27, 2021 14:06
@julfou81
Copy link

Hi, to address this issue with odd number of slices with popup, what is recommended now by FSL is to use a specific configuration file that will not use subsampling and will not be affected by a dimension being an odd number. Of note this will increase the execution time of topup in that case.

https://www.jiscmail.ac.uk/cgi-bin/webadmin?A2=FSL;6c4c9591.2002

Would that be an option in SDCflows to use different configuration files for topup depending on the matrix size of the images?

@oesteban
Copy link
Member

Thanks for the reference - it is pretty good. I don't think, though, the rationale behind such a recommendation applies here:

  1. We are not removing one slice, but adding instead.
  2. There is an extremely strong assumption in Jesper's message that the grids of the template and the target EPI are the same - which is the case when you tailor the protocol to use FSL TOPUP, but from a theoretical point of view might be very problematic.
  3. Those configuration files basically create denser B-Spline basis to account for the design problem of not allowing control points to fall off-grid. Which is IMHO pretty sloppy, and worse, makes results of TOPUP across different matrix sizes uncomparable. I feel it is pretty poor advice. The separation of knots of the B-Spline field has a physical motivation, assuming they are tuned w.r.t. the expected smoothness of the field along each axis.

@julfou81
Copy link

Thank you @oesteban for your quick and detail comments. Very interesting. I did not think about those aspects. I will have to think it through.
On my side I followed Jesper advice for some diffusion data where I had some issue using the slice2vol correction in FSL-eddy when I was changing the number of slices by adding or removing one slice on dataset with odd number of slices.

@oesteban
Copy link
Member

Adding a slice should not affect if appended to the right (as in, correct) end of the axis.

@julfou81
Copy link

ok, sounds good! I will see if I manage to make this work with eddy and eddy_quad doing it right with the slice padding. At the time I made a quick test that did not work and then found Jesper's answer that was pretty convenient for me and did not bother more about changing the number of slice on preprocessing on datasets with odd number of slices. Thank you again for your comments!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants