Repository navigation
Conversation
…ages An ANTsImage with channels_first=True exposes components on axis zero through numpy(). Reconstruction and serialization currently hand that array to a components-last reader. Valid arithmetic therefore raises, and NumPy/pickle round trips silently alter the spatial shape and component count. Move the component axis to the last position before reconstruction and serialization. Preserve channels_first on new_image_like results. NumPy and pickle restore the existing canonical channels-last presentation while preserving physical data and geometry. Add repository-style regression coverage for the corrected behavior.
jig-mistry
left a comment
There was a problem hiding this comment.
I tried this on top of current main. It merges cleanly, the full test suite passes, and with a channels_first=True image, arithmetic, new_image_like, pickle and .npy now give the right shape and data. A few things I noticed while testing:
-
to_nibabel_niftihas the same problem and isn't covered yet. It takesimage.numpy()and treats the last axis as the components (nibabel_nifti_to_ants.py:147), so a channels_first image silently turns into a wrong NIfTI:s = ants.from_numpy(np.random.rand(4, 5, 6).astype('float32')) v = ants.merge_channels([s, s, s], channels_first=True) ants.to_nibabel_nifti(v).shape # (3, 4, 5, 1, 6), should be (4, 5, 6, 1, 3)
-
ANTsImage has no
__deepcopy__, socopy.deepcopyalso goes through__getstate__. With this changedeepcopy(v)has the right data but comes back withchannels_first=False, so itsnumpy()is (4, 5, 6, 3) instead of (3, 4, 5, 6), whilev.clone()keeps the flag. Images sent to multiprocessing workers lose it the same way. Would it work to storechannels_firstas an optional 7th item (only written when it's True) and have__setstate__accept 6 or 7 items? Existing pickles would still load. For.npy, the current reader ignores extra keys in the JSON sidecar, so achannels_firstkey could go there too. -
Probably for a follow-up: a few functions drop the flag, so their output's
numpy()comes back components-last.resample_imagetries to keep it, but the multi-component branch loops withfor image in images:, sochannels_first=image.channels_firston line 75 of resample_image.py reads the flag of the last split channel, which is always False.smooth_imageand the multi-component branch of__getitem__don't passchannels_firsttomerge_channelsat all.
-
Small one: the array is moved before the shape check. So passing a components-last (6, 7, 2) array to
new_image_likeon a (6, 7) channels_first image reportsgiven array shape ((2, 6)) and image array shape ((6, 7)) do not match. That call worked on main, so checking the shape before the move and printing the expectedimage.numpy()shape would make the error easier to understand for anyone who hits it.
It might also be worth adding a deepcopy case and an image-with-image operator (img + img) to the tests.
|
Thanks, @jig-mistry, for the careful testing and concrete reproductions. I pushed follow-up commit
All three affected test modules pass locally (140 passed, 3 skipped, 61 subtests passed). The broader local suite is limited by unrelated transform/native-extension mismatches under the prebuilt 0.6.3 extension; the hosted CI run is in progress. Thanks again for strengthening the patch. |
|
Thanks for the quick follow-up. I checked fca642c on its own and merged into current main, and the full test suite passes on both.
Looks good to me, and keeping the propagation issues for a separate PR makes sense. |
Problem
With
channels_first=True, multicomponentANTsImage.numpy()places the component axis first. Consumers that assume components-last can fail reconstruction or silently alter serialized and NIfTI shapes.Changes
new_image_like, pickle storage,.npywriting, and NIfTI conversion while preserving the in-memory layout.new_image_likeinput against the actualimage.numpy()shape before moving axes and report the received and expected shapes.channels_firstthrough pickle anddeepcopyusing an optional seventh state element; still accept older six-element states.channels_first: truekey in the existing.npyJSON sidecar and restore the flag on read, without changing the saved array layout.to_nibabel_niftito produce(x, y, z, 1, components)with the right data for channels-first 3D vector images.img + img, invalid shapes, legacy pickle state, deepcopy,.npy, and NIfTI round trips.Validation
test_core_ants_transform.py: 278 passed, 3 skipped, 174 subtests passed; 7 failures/subfailures exclusively in unchanged displacement-transform I/O tests. The checked-out Python source calls the native transform-to-field API withNone, which the reused antspyx 0.6.3 native extension does not accept.compileallandgit diff --checkpassed.Compatibility and scope
.npyarrays remain components-last, and older readers can ignore the new optional sidecar key; old sidecars still load with the default channels-last presentation.new_image_likenow requires the documentedimage.numpy()axis layout; manually supplied components-last arrays on a channels-first image may need adaptation.numpy()/view()issues are outside scope.channels_firstpropagation defects inresample_image,smooth_image, and multicomponent indexing are reserved for a follow-up, as suggested in review.