Wrong dimension

[nilgoyette]

Some images that we receive are strangely defined. For example, we have an image

dim: [4, 133, 133, 16, 1, 1, 1, 1]

where nifti-rs thinks that it's a 4D image with dimensions [133, 133, 16, 1] Maybe it's totally domain-related, but here at my job we consider this a 3D image! I tried fixing it myself

let mut header = nifti_object.header().clone();
let mut volume = nifti_object.into_volume();

// Fix bad dimension on some 3D images
if header.dim[header.dim[0] as usize] == 1 {
    header.dim[0] -= 1;
    volume.dim[0] -= 1; // ERROR
}

but, as you know, everything in InMemNiftiVolume is private (as it should). We do use these 3 objects: InMemNiftiVolume -> ArrayD -> Array<D, T> in all kinds of context, so I must fix InMemNiftiVolume, not the others.

What do you think? Should all "false" 4D images considered 3D images? If not, should nifti-rs offers a constructor to fix this? Or something else?

[Enet4]

That is intriguing, but I'm not quite sure that we should change the current behaviour. The specification in the nifti C header file defines dim[0] as the number of dimensions. And so, the library assumes it's 4D because dim[0] is, in fact, 4. If we were to interpret trailing 1's as superfluous and not part of the real dimensions, we might run into situations where the volume was DxHxWx1 by happenstance (worse even, DxHx1x1 or Dx1x1x1), and volume processing pipelines would then have to adapt to different dimensionalities when they were actually just expecting 4D volumes.

With that said, the current state of the plain volume API is a bit unfortunate in the sense that it's quite limited. Multidimensional array libraries usually contain a wide assortment of access and manipulation methods which we do not provide here. The push of ndarray volumes was a way to facilitate integration while mitigating this issue. Nonetheless, you can still get a slice of that 4D volume over the last dimension to make it 3D:

let mut volume = nifti_object.into_volume();
let volume = volume.get_slice(3, 0);

A note of warning though, I recall that into_ndarray on a SliceView is currently not implemented for efficiency.

[nilgoyette]

I agree that we shouldn't change the current behavior. Maybe some images are DxHxWx1 by design for real :) I wasn't aware of get_slice, thank you, but if I can't call into_ndarray on it, it's useless.

This being said, would a standard by-element constructor make sense? You already "use" one in your InMemNiftiVolume tests. (yes, I know you don't need a constructor for the inner tests). Or any method that can mutate the dim array. I'm just throwing ideas because I don't know what could be useful to other programers. I don't want to modify nifti-rs just for my own precious use-case!

[Enet4]

but if I can't call into_ndarray on it, it's useless.

Well, that should still work. If it doesn't, we ought to look into it. Perhaps a few tests on obtaining an ndarray out of a slice view are due.

I agree that there should be a public means to construct volumes (it will be required to write NIfTI volumes from scratch). On the other hand, it might be best for the time being to keep mutating operations away from our NiftiVolume implementations. Instead, there should hopefully be an incentive to convert the volume to a more flexible type (ndarray) and back to a volume if needed, rather than hinting towards making all manipulations in InMemNiftiVolume-land.

An extended slicing API would make sense, though. We could make additional methods for delimiting certain portions of the volume (yielding non-copying views) and optimize those for efficient conversion to an ndarray.

[nilgoyette]

So, if I read you correctly, you suggest

1. "a public means to construct volumes", keeping it immutable
2. volume -> ndarray (modifiable) -> volume

I would go with 1) for now, because it solves my problem by adding a simple function. Is this ok with you? We can still add 2) and an "extended slicing API" later if we ever need them.

[Enet4]

Yes, that's a nice summary. :+1: We can already do 50% of (2), the second half is likely to depend on the construction API. Feel free to propose new constructors for InMemNiftiVolume.

[Enet4]

I think the main concern here has been addressed. Feel free to create new issues to pinpoint other matters mentioned here.