ENH: Add image slicing

[effigies]

See https://mail.python.org/pipermail/neuroimaging/2017-August/001501.html

Closes #489.

[effigies]

@mwaskom Note the slicing... I'm pretty sure the lines I put in the email were dropping one too many voxels in each dimension.

[mwaskom]

This is very nice. It might make sense to expose a slightly more general API that accepts the bounds. In my case I wanted to crop to a volume mask, but others might want to crop based on bound values that they obtain in some other fashion. Does that seem reasonable?

[mwaskom]

Also it might be nice to have a margins (or similar parameter) that will add the number of voxels to pad on each side. In my case I have added that so that I don't lose data if there is a slight registration error between the cropped image and the image I am going to register to it.

[coveralls]

Coverage decreased (-0.04%) to 96.184% when pulling a8af0bc on effigies:enh/crop into a4a667d on nipy:master.

[effigies]

@matthew-brett Want to have a quick look at this? I haven't incorporated @mwaskom's comments, yet, but if this seems reasonable, I can polish it up before the release.

[codecov-io]

Codecov Report

Merging #550 into master will increase coverage by 0.02%.
The diff coverage is 97.5%.

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   90.02%   90.04%   +0.02%     
==========================================
  Files          90       90              
  Lines       11044    11083      +39     
  Branches     1820     1828       +8     
==========================================
+ Hits         9942     9980      +38     
  Misses        770      770              
- Partials      332      333       +1

Impacted Files	Coverage Δ
nibabel/spatialimages.py	96.33% <97.5%> (+0.28%)	⬆️

---

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 1584b3b...919b71a. Read the comment docs.

[effigies]

@mwaskom I made the changes you suggested, if you'd like to have a look. Does it make sense to also include a crop_image_to_mask, which finds the bounds for you and calls crop_image? Or just leave that functionality to users?

[coveralls]

Coverage decreased (-0.04%) to 96.197% when pulling 1ed4537 on effigies:enh/crop into bb8c318 on nipy:master.

[coveralls]

Coverage decreased (-0.04%) to 96.197% when pulling 7ea21c1 on effigies:enh/crop into bb8c318 on nipy:master.

[coveralls]

Coverage decreased (-0.01%) to 96.256% when pulling fb1009e on effigies:enh/crop into dbe74e1 on nipy:master.

[matthew-brett]

Guys - what do you think about pulling over the Nipy image slicing. It would look something like:

cropped = img.slicer[1:-1, 2:-2, ...]

to drop the left and rightmost pixels and 2 posterior and anterior pixels? See: https://github.com/practical-neuroimaging/analysis-clinic/blob/master/nipy_image_slicing.ipynb

[satra]

@matthew-brett - i think being able to slice would be nice. my worry is that this affords "easy" but potentially incorrect downsampling, and that users really should know how the image was acquired in the scanner (line, epi, 3d, etc.,.) before applying the operation.

see: https://mail.python.org/pipermail/neuroimaging/2017-September/001528.html

[matthew-brett]

@satra - do you mean, you think we shouldn't implement this in case people mis-use it? I guess we can make them do it by hand? Or would you be OK with a big warning in the docstring?

[satra]

@matthew-brett - it's more a worry than me saying we shouldn't implement this.

slicing is so easy in numpy that as soon as you have the data in a numeric array, people will end up doing it. and in many cases they may have done the right thing (smoothing/filtering) prior to the slicing step. so we shouldn't take away easy slicing for that reason. the key that this functionality adds is the adjustment of headers and other things to reflect the outcome of that operation.

putting a note in the docstring is a good starting point.

[mwaskom]

If people are going to have to call a filtering function anyway, it seems to erase the convenience of having the slice notation. At that point, why not just have one function that filters and downsamples?

[effigies]

Using slices for cropping would be fine with me, but I think I'm less comfortable with adding downsampling. That makes more sense to me as a function where options like filtering can be specified.

I like the nipy image approach, but I also like that nibabel added enough friction to make me think about whether I wanted to switch to nipy to use it, or use some other tool to filter.

[effigies]

Before I put work into this, my approach would be to only permit slices and ellipses, and constrain slices not to specify step (except 1 or None). I'd allow cropping in the temporal dimension as well, since that's trivial.

This would be in SpatialImage, but I think the affine update step would need to be overridden in images whose first three dimensions aren't spatial.

Seem reasonable?

[satra]

@effigies - sounds good to me.

if there is a function that's being created to support a functional form one could potentially allow something like this:

img.slice(..., pre_filtered=True)

under that setting you can allow steps.

[effigies]

@satra IMO, by talking actual filtering, we're getting into nipy/nilearn territory (possibly nistats? Not sure, haven't looked closely enough yet). Obviously there's a blurry line, but my idea of nibabel's scope is just wrangling data formats, while leaving stats and signal processing to other tools.

I don't think it would be good to get into a position where users have to make a choice between downsampling in the high-level or low-level package, and consider the cascading effects of that decision. If it's needed here, then I think we'll want broad input on how to implement it so that higher level libraries use our algorithms, rather than offer alternatives.

@matthew-brett I'm going to declare down-sampling out-of-scope for this PR, but I'm okay with adding bounding slices. And I'm okay with allowing img[[0], ...] but not img[0, ...], since the former keeps the dimensions intact while being consistent with numpy indexing, but the latter has to sacrifice numpy consistency for coherency as a SpatialImage.

@mwaskom This PR was kicked off by your idea. Would SpatialImage.__getitem__ be a sufficient interface, or should funcs.crop_image stay around? The bounding behavior of __getitem__ would need to be different from what I currently have (using inclusive bounds), and would require the user to calculate margins.

[satra]

@effigies - how about the following keyword instead allow_downsampling=True with False being the default. the intent here is to allow regular options for slicing including steps, but it serves as a point to indicate to the potential user that this means something specific.

[matthew-brett]

Hum . So this is something that I often do for various reasons so I would miss it if I could not do it. I think we need to think of use cases. I often do this when I want a quick and dirty smaller version of a high resolution file. Our I'm going to smooth it later down the line. Of course this can also be used to subsample in time and other dimensions. Would you propose trying to work out the optimal smoothing by default? Also in time etc?

…

On 27 Sep 2017 15:49, "Satrajit Ghosh" ***@***.***> wrote: @effigies <https://github.com/effigies> - how about the following keyword instead allow_downsampling=True with False being the default. the intent here is to allow regular options for slicing including steps, but it serves as a point to indicate to the potential user that this means something specific. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#550 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEIHFYr1A8PEwpfdLj0nIqumataq9tLks5smmB0gaJpZM4O79Wz> .

[satra]

@matthew-brett - that's why i propose having the keyword (allow_downsampling) - precisely for those situations where you know what you are doing, whether for quick and dirty, or my data are band-limited.

there are use cases where downsampling is never an issue and smoothing/filtering does not need to be used. for example, with slice removal (if the same slices are removed for all volumes)

[satra]

@matthew-brett - regarding the comment of trying to figure out optimal filters, i would stay away from that in nibabel, unless nibabel wanted to have utilities to show power spectrum (space/time), etc.,.

[effigies]

I guess I'm not sure what API you're proposing.

class SpatialImage(...):
    def slice(*args, allow_downsampling=False):
        ...

What are *args, and what's the effect of allow_downsampling? Don't raise an exception when someone tries to slice with a non-1 step? My biggest issue here is just that the syntactic sugar for slicing is only available to __getitem__, which can't take these sorts of arguments.

You could do something like:

class UnsafeSlicer:
    def __init__(self, img):
        self.img = img

    def __getitem__(self, idx):
        dataobj = self.img.dataobj[idx]
        affine = self.img.slice_affine(idx)
        return self.img.__class__(dataobj, affine, self.img.header)

Which could then be called:

new_img = UnsafeSlicer(img)[::2, -20:-2:3, ..., [0]]

The main thing that would need to be added is a method that returns a modified affine with bounds updates and subsampling.

[satra]

@effigies - perhaps something like this.

class SpatialImage(...):
    def slice(*args, allow_downsampling=False):
        ...
    def __getitem__(self, idx):
        if requires_downsampling(idx):
            raise ValueError('slicing requires downsampling, please use img.slice(..., allow_downsampling=True) instead')
        dataobj = self.img.dataobj[idx]
        affine = self.img.slice_affine(idx)
        return self.img.__class__(dataobj, affine, self.img.header)

and *args would be exactly the same as idx in __getitem__

[effigies]

@satra Okay, that seems manageable.

[effigies]

@satra @matthew-brett Let me know what you think of this API.

[effigies]

I need to check that this isn't backwards.

[effigies]

@matthew-brett Changes made, if you've got time for another review.

[matthew-brett]

Just a couple of comments.

[matthew-brett]

Can this go at the top of the file now? Or would that make a circular import? If so, maybe worth a comment.

[matthew-brett]

Maybe worth a comment here about this line being specific to spatial-first images. Or generalize somehow to allow overriding and working out spatial slices from the image.

[matthew-brett]

Thanks for your patience, in it goes ...