ENH: Add nibabel-based split and merge interfaces #489

dPys · 2020-03-24T20:38:55Z

per suggestion here

pull-assistant · 2020-03-24T20:38:57Z

Best reviewed: commit by commit

Optimal code review plan (5 warnings)

[ENH] Add nibabel-based split and merge interfaces per https://github....

...orkflows/interfaces/nibabel.py 66% changes removed in fix: apply review co...

Update niworkflows/interfaces/nibabel.py

Chage naming to ConcatImages, fix in/out spec namings

...orkflows/interfaces/nibabel.py 89% changes removed in enh: make i/o specs ...

Update niworkflows/interfaces/nibabel.py

rename ConcatImages to MergeSeries, correct typo in description of out...

Update niworkflows/interfaces/nibabel.py

Apply suggestions from code review [skip ci]

...orkflows/interfaces/nibabel.py 50% changes removed in fix: apply review co...

fix: added few bugfixes and regression tests

...terfaces/tests/test_nibabel.py 42% changes removed in fix: apply review co...

fix: squeeze image with np.squeeze / change input name for consistency

...terfaces/tests/test_nibabel.py 50% changes removed in fix: apply review co...

sty(black): standardize formatting a bit

enh: make i/o specs of SplitSeries more consistent [skip ci]

Update niworkflows/interfaces/tests/test_nibabel.py [skip ci]

fix: apply review comments from @effigies, add parameterized tests

Powered by Pull Assistant. Last update 0de7374 ... d657546. Read the comment docs.

niworkflows/interfaces/nibabel.py

pep8speaks · 2020-03-24T21:56:21Z

Hello @dPys, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2020-04-10 22:47:03 UTC

niworkflows/interfaces/nibabel.py

oesteban

Looking good! Left a few comments.

niworkflows/interfaces/nibabel.py

oesteban

It'd be great to get this pushed in sometime soon :)

niworkflows/interfaces/nibabel.py

oesteban · 2020-04-08T00:37:20Z

BTW, in the interest of your time, I don't mind taking over the thrust in this PR, @dPys - let me know if you are fine with me editing your code

dPys · 2020-04-08T01:42:03Z

Go for it @oesteban :-)

oesteban · 2020-04-08T21:50:12Z

@effigies, should squeeze_image not remove axes from data? I'm creating a test with data of shape (20, 20, 20, 1, 3) and I would expect that would be squeezed to (20, 20, 20, 3) but it doesn't seem to be the case:

        # Test splitting ANTs warpfields
        data = np.ones((20, 20, 20, 1, 3), dtype=float)
        in_file = tmp_path / 'warpfield.nii.gz'
        nb.Nifti1Image(data, np.eye(4), None).to_filename(str(in_file))
    
>       split = SplitSeries(in_file=str(in_file)).run()
/home/travis/build/nipreps/niworkflows/niworkflows/interfaces/tests/test_nibabel.py:124: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/tmp/venv/lib/python3.6/site-packages/nipype/interfaces/base/core.py:397: in run
    runtime = self._run_interface(runtime)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <niworkflows.interfaces.nibabel.SplitSeries object at 0x7fd69a6c6cc0>
runtime = Bunch(cwd='/tmp/pytest-of-travis/pytest-0/test_SplitSeries0', duration=0.003802, endTime='2020-04-08T20:10:06.709906',...s 5D (20x20x20x1x3).\nAn exception of type RuntimeError occurred while running interface SplitSeries.',), version=None)
    def _run_interface(self, runtime):
        filenii = nb.squeeze_image(nb.load(self.inputs.in_file))
        ndim = filenii.dataobj.ndim
        if ndim != 4:
            if self.inputs.accept_3D and ndim == 3:
                out_file = str(
                    Path(fname_presuffix(self.inputs.in_file, suffix=f"_idx-000")).absolute()
                )
                self._results['out_files'] = out_file
                filenii.to_filename(out_file)
                return runtime
            raise RuntimeError(
>               f"Input image image is {ndim}D ({'x'.join(['%d' % s for s in filenii.shape])}).")
E           RuntimeError: Input image image is 5D (20x20x20x1x3).
/home/travis/build/nipreps/niworkflows/niworkflows/interfaces/nibabel.py:123: RuntimeError

effigies · 2020-04-08T22:00:23Z

It removes final 1-dimensions. Check the docstring.

…ep#77

Co-Authored-By: Chris Markiewicz <[email protected]>

…tputs

Co-Authored-By: Oscar Esteban <[email protected]>

codecov · 2020-04-08T23:02:44Z

Codecov Report

Merging #489 into master will increase coverage by 9.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   49.46%   58.89%   +9.43%     
==========================================
  Files          42       43       +1     
  Lines        5093     5603     +510     
  Branches      741      883     +142     
==========================================
+ Hits         2519     3300     +781     
+ Misses       2473     2144     -329     
- Partials      101      159      +58

Flag	Coverage Δ
#documentation	`33.68% <42.85%> (+0.04%)`	⬆️
#reportlettests	`100.00% <ø> (ø)`
#travis	`58.09% <100.00%> (?)`
#unittests	`46.21% <100.00%> (+0.48%)`	⬆️

Impacted Files	Coverage Δ
niworkflows/interfaces/nibabel.py	`100.00% <100.00%> (ø)`
niworkflows/viz/__init__.py	`100.00% <0.00%> (ø)`
niworkflows/viz/notebook.py	`23.80% <0.00%> (ø)`
niworkflows/reports/core.py	`72.25% <0.00%> (+0.16%)`	⬆️
niworkflows/interfaces/freesurfer.py	`49.41% <0.00%> (+0.39%)`	⬆️
niworkflows/utils/spaces.py	`89.50% <0.00%> (+0.48%)`	⬆️
niworkflows/interfaces/registration.py	`51.72% <0.00%> (+3.44%)`	⬆️
niworkflows/interfaces/plotting.py	`84.74% <0.00%> (+10.16%)`	⬆️
niworkflows/interfaces/mni.py	`42.52% <0.00%> (+16.66%)`	⬆️
niworkflows/interfaces/masks.py	`62.13% <0.00%> (+18.34%)`	⬆️
... and 4 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 e43c6c1...d657546. Read the comment docs.

oesteban

Okay, I think this is complete. I have beefed up the tests and addressed some minor things here and there.

niworkflows/interfaces/tests/test_nibabel.py

effigies · 2020-04-09T00:45:47Z

niworkflows/interfaces/nibabel.py

+        filenii = nb.squeeze_image(nb.load(self.inputs.in_file))
+        filenii = filenii.__class__(
+            np.squeeze(filenii.dataobj), filenii.affine, filenii.header
+        )


The reason that non-terminal 1-dimensions are left in place by squeeze_image is to preserve the meaning of each dimension. For example, if I have a time series of a single slice (64, 64, 1, 48), squeeze_image will preserve the meaning of i, j, k, n, but np.squeeze will recast n as k.

What's the use case that you're taking care of here?

Yes, the use case I'm contemplating is separating out the three components of deformation fields and other model-based nonlinear transforms. There is one example of this in the tests.

Found it. I guess I would think splitting along the fifth dimension is a different task from splitting along the fourth, but I see that it's convenient to have a single interface. I'm not sure that risking dropping meaningful dimensions is a good idea.

What about forcing to 4D like:

extra_dims = tuple(dim for dim in img.shape[3:] if dim > 1) or (1,) if len(extra_dims) != 1: raise ValueError("Invalid shape") img = img.__class__(img.dataobj.reshape(img.shape[:3] + extra_dims), img.affine, img.header)

This coerces a 3D image to (x, y, z, 1) and a 4+D image to (x, y, z, n) assuming that dimensions 4-7 are all 1, n or absent.

Is it guaranteed that the spatial dimensions will not be affected on that reshape? If so, I'm down with this solution.

Trivial dimensions have no effect on indexing, whether it's C- or Fortran ordered. If you don't change the order, it's fine.

Co-Authored-By: Chris Markiewicz <[email protected]>

effigies · 2020-04-09T12:19:17Z

niworkflows/interfaces/nibabel.py

+        )
+        ndim = filenii.dataobj.ndim
+        if ndim != 4:
+            if self.inputs.allow_3D and ndim == 3:


This is odd, as the above will coerce a valid 4D (x, y, z, 1) image to 3D (x, y, z), requiring you to then allow_3D.

why is that odd? It is indeed a 3D volume, right?

for instance, there are a fair number of T1w images in OpenNeuro with (x, y, z, 1) dimensions.

It is a 4D series. You should be able to split it into one 3D volume without special casing it.

I guess the above snippet you wrote would make this particular use-case a standard one?

I would say checking for 3D volumes should happen before reshaping the image.

effigies

A full enumeration of input shapes and expected outputs for each interface would be a good idea. I feel like this is going to have annoying edge cases and we're not testing them.

Some cases for split:

Spatial dimensions of length 1
Non-spatial dimensions of length 1 with and without allow_3d
Multiple non-spatial dimensions of length >1
Various non-spatial dimensions of length >1 with other non-spatial dimensions of length 1

Merge I think should be simpler to come up with cases for (more invalid cases), but making sure we understand the dynamics is a good idea with so much squeezing going on.

I'm also not really sure that these are "nibabel-based" at this point.

oesteban

A full enumeration of input shapes and expected outputs for each interface would be a good idea. I feel like this is going to have annoying edge cases and we're not testing them.

Some cases for split:

Spatial dimensions of length 1
Non-spatial dimensions of length 1 with and without allow_3d
Multiple non-spatial dimensions of length >1
Various non-spatial dimensions of length >1 with other non-spatial dimensions of length 1
Merge I think should be simpler to come up with cases for (more invalid cases), but making sure we understand the dynamics is a good idea with so much squeezing going on.

How much of this you think would be left uncovered after I insert your snippet for the split?

I'm also not really sure that these are "nibabel-based" at this point.

Sure, is there any practical consequence of this? Happy to change the name of the module to something broader. However, the ApplyMask and Binarize will need to maintain some aliases in that case for a few releases.

oesteban · 2020-04-09T16:30:09Z

niworkflows/interfaces/nibabel.py

+        filenii = nb.squeeze_image(nb.load(self.inputs.in_file))
+        filenii = filenii.__class__(
+            np.squeeze(filenii.dataobj), filenii.affine, filenii.header
+        )


Is it guaranteed that the spatial dimensions will not be affected on that reshape? If so, I'm down with this solution.

oesteban · 2020-04-09T16:31:41Z

niworkflows/interfaces/nibabel.py

+        )
+        ndim = filenii.dataobj.ndim
+        if ndim != 4:
+            if self.inputs.allow_3D and ndim == 3:


I guess the above snippet you wrote would make this particular use-case a standard one?

effigies · 2020-04-09T17:12:10Z

How much of this you think would be left uncovered after I insert your snippet for the split?

I think it should work fine, but the point was more that we should probably do this in a test-driven way.

Sure, is there any practical consequence of this?

No. I actually meant to delete it. I was going to say "If the idea is to be making nibabel functions available as interfaces, we've definitely strayed from that goal." But then I decided it didn't matter and failed to remove it.

effigies

Thanks for your patience. This LGTM.

Addresses an oversight in #489 .

dPys mentioned this pull request Mar 24, 2020

[ENH] Add emc-related helper functions - images nipreps/dmriprep#77

Open

effigies reviewed Mar 24, 2020

View reviewed changes

effigies reviewed Mar 31, 2020

View reviewed changes

niworkflows/interfaces/nibabel.py Outdated Show resolved Hide resolved

niworkflows/interfaces/nibabel.py Outdated Show resolved Hide resolved

niworkflows/interfaces/nibabel.py Outdated Show resolved Hide resolved

oesteban reviewed Apr 1, 2020

View reviewed changes

oesteban reviewed Apr 8, 2020

View reviewed changes

oesteban requested a review from mgxd April 8, 2020 22:11

oesteban changed the title ~~[ENH] Add nibabel-based split and merge interfaces~~ ENH Add nibabel-based split and merge interfaces Apr 8, 2020

oesteban changed the title ~~ENH Add nibabel-based split and merge interfaces~~ ENH: Add nibabel-based split and merge interfaces Apr 8, 2020

dPys and others added 16 commits April 8, 2020 15:12

[ENH] Add nibabel-based split and merge interfaces per nipreps/dmripr…

0de7374

…ep#77

Update niworkflows/interfaces/nibabel.py

bfcd29c

Co-Authored-By: Chris Markiewicz <[email protected]>

Update niworkflows/interfaces/nibabel.py

30ddf03

Co-Authored-By: Chris Markiewicz <[email protected]>

Update niworkflows/interfaces/nibabel.py

5e3c93c

Co-Authored-By: Chris Markiewicz <[email protected]>

Update niworkflows/interfaces/nibabel.py

40f6de1

Co-Authored-By: Chris Markiewicz <[email protected]>

Chage naming to ConcatImages, fix in/out spec namings

f3572e6

Update niworkflows/interfaces/nibabel.py

49ff6bd

Co-Authored-By: Chris Markiewicz <[email protected]>

Update niworkflows/interfaces/nibabel.py

87f1cb3

Co-Authored-By: Chris Markiewicz <[email protected]>

rename ConcatImages to MergeSeries, correct typo in description of ou…

b4fcdb7

…tputs

Update niworkflows/interfaces/nibabel.py

f51f510

Co-Authored-By: Oscar Esteban <[email protected]>

Update niworkflows/interfaces/nibabel.py

9aa0655

Co-Authored-By: Oscar Esteban <[email protected]>

Update niworkflows/interfaces/nibabel.py

5a31b01

Co-Authored-By: Oscar Esteban <[email protected]>

Update niworkflows/interfaces/nibabel.py

53db81a

Co-Authored-By: Oscar Esteban <[email protected]>

Update niworkflows/interfaces/nibabel.py

1c27f20

Co-Authored-By: Oscar Esteban <[email protected]>

Update niworkflows/interfaces/nibabel.py

05724d5

Co-Authored-By: Oscar Esteban <[email protected]>

Apply suggestions from code review [skip ci]

7263d03

oesteban added 3 commits April 8, 2020 15:12

fix: added few bugfixes and regression tests

03ebb6d

fix: squeeze image with np.squeeze / change input name for consistency

9446d47

sty(black): standardize formatting a bit

0ae5351

oesteban force-pushed the enh/nibabel_splitmerge_interfaces branch from acd9b67 to 0ae5351 Compare April 8, 2020 22:13

oesteban requested a review from effigies April 8, 2020 23:09

enh: make i/o specs of SplitSeries more consistent [skip ci]

fc42351

oesteban approved these changes Apr 8, 2020

View reviewed changes

effigies reviewed Apr 9, 2020

View reviewed changes

Update niworkflows/interfaces/tests/test_nibabel.py [skip ci]

1d12dd3

Co-Authored-By: Chris Markiewicz <[email protected]>

effigies reviewed Apr 9, 2020

View reviewed changes

oesteban reviewed Apr 9, 2020

View reviewed changes

fix: apply review comments from @effigies, add parameterized tests

d657546

oesteban requested a review from effigies April 10, 2020 22:50

effigies approved these changes Apr 16, 2020

View reviewed changes

oesteban merged commit 7b28b4f into nipreps:master Apr 16, 2020

oesteban added a commit that referenced this pull request Apr 22, 2020

FIX: Merge/SplitSeries write to path of input image, instead of cwd

1fc518b

Addresses an oversight in #489 .

oesteban mentioned this pull request Apr 22, 2020

FIX: Merge/SplitSeries write to path of input image, instead of cwd #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add nibabel-based split and merge interfaces #489

ENH: Add nibabel-based split and merge interfaces #489

dPys commented Mar 24, 2020

pull-assistant bot commented Mar 24, 2020 •

edited

Loading

pep8speaks commented Mar 24, 2020 •

edited

Loading

oesteban left a comment

oesteban left a comment

oesteban commented Apr 8, 2020

dPys commented Apr 8, 2020

oesteban commented Apr 8, 2020

effigies commented Apr 8, 2020

codecov bot commented Apr 8, 2020 •

edited

Loading

oesteban left a comment

effigies Apr 9, 2020

oesteban Apr 9, 2020

effigies Apr 9, 2020

oesteban Apr 9, 2020

effigies Apr 9, 2020

effigies Apr 9, 2020

oesteban Apr 9, 2020

oesteban Apr 9, 2020

effigies Apr 9, 2020

oesteban Apr 9, 2020

effigies Apr 9, 2020

effigies left a comment

oesteban left a comment

oesteban Apr 9, 2020

oesteban Apr 9, 2020

effigies commented Apr 9, 2020

effigies left a comment

ENH: Add nibabel-based split and merge interfaces #489

ENH: Add nibabel-based split and merge interfaces #489

Conversation

dPys commented Mar 24, 2020

pull-assistant bot commented Mar 24, 2020 • edited Loading

pep8speaks commented Mar 24, 2020 • edited Loading

Comment last updated at 2020-04-10 22:47:03 UTC

oesteban left a comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

oesteban commented Apr 8, 2020

dPys commented Apr 8, 2020

oesteban commented Apr 8, 2020

effigies commented Apr 8, 2020

codecov bot commented Apr 8, 2020 • edited Loading

Codecov Report

oesteban left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Apr 9, 2020

effigies left a comment

Choose a reason for hiding this comment

pull-assistant bot commented Mar 24, 2020 •

edited

Loading

pep8speaks commented Mar 24, 2020 •

edited

Loading

codecov bot commented Apr 8, 2020 •

edited

Loading