Enh/wip/fix/brk topup for odd dimensions. help needed #2832

akeshavan · 2018-12-19T02:32:03Z

Summary

Attempts to fix #948 -- its 4 years later and we are still having this problem! I slightly modified @oesteban 's original commit with a flag called checksize, which, if set to True will make the size even before running TOPUP and ApplyTOPUP. It's set to False by default.

Problem is, the ApplyTOPUP interface doesn't work!! The process just gets killed. Someone help please!

List of changes proposed in this PR (pull-request)

fix the ApplyTOPUP interface

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

fix: completed outputspec for EddyQuad

fix: made the outputdir be mandatory and use the default val

- Remove redundant `__init__` method in `EddyQuad` class. - Use `os.path.abspath()` earlier in order to remove from later statements. - Improve `EddyQuad`'s `base_name` input description. - Remove unnecessary `mandatory=False` params. - Rename `slspec` to `slice_spec`. - Use default for `EddyQuad`'s `base_name` input. - Use a name template for `EddyQuad`'s `output_dir` input.

akeshavan · 2018-12-19T04:53:24Z

Nevermind about the applytopup, it turns out I was testing this on a pretty large file :) thanks @oesteban for the help!

codecov-io · 2018-12-19T05:04:49Z

Codecov Report

Merging #2832 into master will increase coverage by 0.09%.
The diff coverage is 18.82%.

@@            Coverage Diff             @@
##           master    #2832      +/-   ##
==========================================
+ Coverage   67.45%   67.54%   +0.09%     
==========================================
  Files         341      341              
  Lines       43355    44091     +736     
  Branches     5379     5626     +247     
==========================================
+ Hits        29245    29783     +538     
- Misses      13413    13592     +179     
- Partials      697      716      +19

Flag	Coverage Δ
#smoketests	`50.11% <15.29%> (-0.44%)`	⬇️
#unittests	`65.03% <18.82%> (+0.18%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/fsl/epi.py	`56.2% <18.82%> (-8.01%)`	⬇️
nipype/workflows/fmri/spm/preprocess.py	`70.4% <0%> (-2.05%)`	⬇️
nipype/utils/filemanip.py	`78.07% <0%> (-1.58%)`	⬇️
nipype/utils/provenance.py	`83.43% <0%> (-1.28%)`	⬇️
nipype/pipeline/engine/nodes.py	`84.19% <0%> (-0.34%)`	⬇️
nipype/utils/nipype_cmd.py	`44.61% <0%> (+1.99%)`	⬆️
nipype/interfaces/base/core.py	`88.88% <0%> (+2.07%)`	⬆️
nipype/utils/profiler.py	`22.04% <0%> (+2.61%)`	⬆️
nipype/interfaces/base/support.py	`82.54% <0%> (+2.89%)`	⬆️
nipype/algorithms/confounds.py	`69.84% <0%> (+3.23%)`	⬆️
... and 2 more

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 2b124a9...b6eaed8. Read the comment docs.

codecov-io · 2018-12-19T05:04:50Z

Codecov Report

Merging #2832 into master will decrease coverage by 0.06%.
The diff coverage is 70.46%.

@@            Coverage Diff             @@
##           master    #2832      +/-   ##
==========================================
- Coverage    66.9%   66.84%   -0.07%     
==========================================
  Files         341      342       +1     
  Lines       43454    43648     +194     
  Branches     5392     5436      +44     
==========================================
+ Hits        29074    29175     +101     
- Misses      13653    13741      +88     
- Partials      727      732       +5

Flag	Coverage Δ
#smoketests	`50.39% <29.52%> (-0.08%)`	⬇️
#unittests	`63.99% <71.42%> (-0.27%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/ants/__init__.py	`100% <ø> (ø)`	⬆️
nipype/workflows/dmri/fsl/artifacts.py	`80.39% <ø> (ø)`	⬆️
nipype/interfaces/nipy/preprocess.py	`45.79% <ø> (ø)`	⬆️
nipype/pipeline/plugins/legacymultiproc.py	`61.5% <ø> (ø)`	⬆️
nipype/interfaces/petpvc.py	`54.71% <ø> (ø)`	⬆️
nipype/interfaces/dipy/preprocess.py	`23.52% <ø> (ø)`	⬆️
nipype/interfaces/dipy/simulate.py	`23.59% <ø> (ø)`	⬆️
nipype/interfaces/dcm2nii.py	`48.13% <ø> (+0.93%)`	⬆️
nipype/interfaces/spm/preprocess.py	`56.7% <0%> (+0.21%)`	⬆️
nipype/pipeline/engine/workflows.py	`78.95% <100%> (ø)`	⬆️
... and 70 more

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 04ba94d...87e5361. Read the comment docs.

effigies · 2018-12-19T15:04:06Z

nipype/interfaces/fsl/epi.py

+        sizes = np.array(data.shape[:3])
+
+        if self._offsets is None:
+            self._offsets = np.array(sizes % 2)


Since sizes is an array, this should already be an array.

Suggested change

self._offsets = np.array(sizes % 2)

self._offsets = sizes % 2

effigies · 2018-12-19T16:03:30Z

nipype/interfaces/fsl/epi.py

+    def _cropfile(self, fname, postfix=''):
+        import nibabel as nb
+        im = nb.load(fname)
+        data = im.get_data()


get_data() is deprecated, but we can get away with not directly loading it. See below.

effigies · 2018-12-19T16:16:21Z

nipype/interfaces/fsl/epi.py

+        if self._offsets is None:
+            self._offsets = np.array(sizes % 2)
+
+        if np.any(self._offsets == 1):


A lot of the work in here is built into nibabel and nipype, so you can reduce this quite a lot:

What about this:

if self._offsets.any(): cropped = im.slicer[:self._offsets[0], :self._offsets[1], :self._offsets[2]] # Remembering to import fname_presuffix from ..utils.filemanip out_fname = fname_presuffix(fname, suffix='_cropped' + postfix, newpath=os.getcwd()) cropped._to_filename(out_fname) else: out_fname = fname return out_fname

effigies · 2018-12-19T16:19:10Z

nipype/interfaces/fsl/epi.py

+        s = imdata.shape
+        dests = np.array(s)
+        dests[:3] = s[:3] + self._offsets
+        data = np.zeros(dests, dtype=im.get_data_dtype())


Suggested change

data = np.zeros(dests, dtype=im.get_data_dtype())

data = np.zeros(dests, dtype=imdata.dtype)

effigies · 2018-12-20T01:09:31Z

nipype/interfaces/fsl/epi.py

+        dests = np.array(s)
+        dests[:3] = s[:3] + self._offsets
+        data = np.zeros(dests, dtype=im.get_data_dtype())
+        data[0:s[0], 0:s[1], 0:s[2], ...] = imdata


This can be a little briefer:

Suggested change

data[0:s[0], 0:s[1], 0:s[2], ...] = imdata

data[:s[0], :s[1], :s[2]] = imdata

effigies · 2018-12-20T01:49:41Z

nipype/interfaces/fsl/epi.py

+            fixfname, ext2 = op.splitext(fixfname)
+            ext = ext2 + ext
+
+        fixfname = op.abspath(fixfname + '_fix' + ext)


You can do all of this in one line:

fixfname = fname_presuffix(fname, suffix='_fix', newpath=os.getcwd())

effigies · 2018-12-20T01:50:14Z

nipype/interfaces/fsl/epi.py

+
+        fixfname = op.abspath(fixfname + '_fix' + ext)
+
+        hdr = im.get_header().copy()


No need to manually set the header. Shape gets overwritten by the actual data array shape.

Suggested change

hdr = im.get_header().copy()

effigies · 2018-12-20T01:50:24Z

nipype/interfaces/fsl/epi.py

+        fixfname = op.abspath(fixfname + '_fix' + ext)
+
+        hdr = im.get_header().copy()
+        hdr.set_data_shape(data.shape)


Suggested change

hdr.set_data_shape(data.shape)

effigies · 2018-12-20T01:51:11Z

nipype/interfaces/fsl/epi.py

+
+        hdr = im.get_header().copy()
+        hdr.set_data_shape(data.shape)
+        nb.Nifti1Image(data, im.get_affine(), hdr).to_filename(fixfname)


get_affine is deprecated, and we can make this work with non-NIfTI-1 images:

Suggested change

nb.Nifti1Image(data, im.get_affine(), hdr).to_filename(fixfname)

img.__class__(data, im.affine, im.header).to_filename(fixfname)

effigies · 2018-12-20T02:09:58Z

nipype/interfaces/fsl/epi.py

+        for name, spec in sorted(self.inputs.traits(**check).items()):
+            if spec.checksize and self.inputs.checksize:
+                value = self._checksize(name)
+                setattr(self.inputs, name, value)


Is this going to cause it to be rerun every time? I would verify.

…fix_topup

mattcieslak · 2019-08-05T22:09:24Z

Would you consider adding a file dependency? There is a second cnf that works for volumes with odd numbers of slices.

here's b02b0_1.cnf from the fsl mailing list

# Resolution (knot-spacing) of warps in mm
--warpres=20,16,14,12,10,6,4,4,4
# Subsampling level (a value of 2 indicates that a 2x2x2 neighbourhood is collapsed to 1 voxel)
--subsamp=1,1,1,1,1,1,1,1,1
# FWHM of gaussian smoothing
--fwhm=8,6,4,3,3,2,1,0,0
# Maximum number of iterations
--miter=5,5,5,5,5,10,10,20,20
# Relative weight of regularisation
--lambda=0.0005,0.0001,0.00001,0.0000015,0.0000005,0.0000005,0.00000005,0.0000000005,0.00000000001
# If set to 1 lambda is multiplied by the current average squared difference
--ssqlambda=1
# Regularisation model
--regmod=bending_energy
# If set to 1 movements are estimated along with the field
--estmov=1,1,1,1,1,0,0,0,0
# 0=Levenberg-Marquardt, 1=Scaled Conjugate Gradient
--minmet=0,0,0,0,0,1,1,1,1
# Quadratic or cubic splines
--splineorder=3
# Precision for calculation and storage of Hessian
--numprec=double
# Linear or spline interpolation
--interp=spline
# If set to 1 the images are individually scaled to a common mean intensity 
--scale=1

oesteban and others added 12 commits October 10, 2014 16:53

Topup inputs check. Close nipy#948

4616c73

Add EddyQuad interface

516e84d

fix: completed outputspec for EddyQuad

6465247

Merge pull request #1 from akeshavan/enh/eddy_quad

9ead4cf

fix: completed outputspec for EddyQuad

fix: made the outputdir be mandatory and use the default val

7b5e7ab

Merge pull request nipy#2 from akeshavan/enh/eddy_quad

0261aab

fix: made the outputdir be mandatory and use the default val

Add doctest for EddyQuad

149ddfd

Add name to .zenodo.json

f59feb0

Fix tests for EddyQuad in interfaces/fsl/epi.py

dbbb84b

wip: incoroporated oscar's topup fix and added a flag to turn it on

70dc61e

added forgotten test

b6eaed8

akeshavan mentioned this pull request Dec 19, 2018

TOPUP and ApplyTOPUP crash with images with odd number of pixels #948

Open

effigies reviewed Dec 20, 2018

View reviewed changes

Adam Richie-Halford and others added 9 commits January 18, 2019 13:56

Add glob stuff back in

69d8e0d

fix: PR suggestions

578a550

Merge remote-tracking branch 'adam/fsl-eddy-quad-interface' into enh/…

6c908a2

…fix_topup

Remove redundant out_avg_b_png lines

03f2b07

Add output_dir check to EddyQuad._list_outputs()

35bc89c

fix: added TOPUPBase to other interfaces that need the checksize method

8ba1dbf

Merge remote-tracking branch 'adam/fsl-eddy-quad-interface' into enh/…

f9d33c4

…fix_topup

wip: addressing code review

04ba94d

fix: merge conflicts

87e5361

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enh/wip/fix/brk topup for odd dimensions. help needed #2832

Enh/wip/fix/brk topup for odd dimensions. help needed #2832

akeshavan commented Dec 19, 2018 •

edited

Loading

akeshavan commented Dec 19, 2018

codecov-io commented Dec 19, 2018

codecov-io commented Dec 19, 2018 •

edited by codecov bot

Loading

effigies Dec 19, 2018

effigies Dec 19, 2018

effigies Dec 19, 2018

effigies Dec 19, 2018

effigies Dec 20, 2018

effigies Dec 20, 2018

effigies Dec 20, 2018

effigies Dec 20, 2018

effigies Dec 20, 2018

effigies Dec 20, 2018

mattcieslak commented Aug 5, 2019

	self._offsets = np.array(sizes % 2)
	self._offsets = sizes % 2

	data = np.zeros(dests, dtype=im.get_data_dtype())
	data = np.zeros(dests, dtype=imdata.dtype)

	data[0:s[0], 0:s[1], 0:s[2], ...] = imdata
	data[:s[0], :s[1], :s[2]] = imdata


		fixfname = op.abspath(fixfname + '_fix' + ext)

		hdr = im.get_header().copy()

	nb.Nifti1Image(data, im.get_affine(), hdr).to_filename(fixfname)
	img.__class__(data, im.affine, im.header).to_filename(fixfname)

Enh/wip/fix/brk topup for odd dimensions. help needed #2832

Are you sure you want to change the base?

Enh/wip/fix/brk topup for odd dimensions. help needed #2832

Conversation

akeshavan commented Dec 19, 2018 • edited Loading

Summary

List of changes proposed in this PR (pull-request)

Acknowledgment

akeshavan commented Dec 19, 2018

codecov-io commented Dec 19, 2018

Codecov Report

codecov-io commented Dec 19, 2018 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcieslak commented Aug 5, 2019

akeshavan commented Dec 19, 2018 •

edited

Loading

codecov-io commented Dec 19, 2018 •

edited by codecov bot

Loading