AsChannelLast/First to support multiple dims

[wyli]

Signed-off-by: Wenqi Li wenqil@nvidia.com

Description

depends on #3648

this will be useful for handling the nifti format, for example to move the time/channel dimensions together

Status

Ready

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
• Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
• In-line docstrings updated.
• Documentation updated, tested make html command in the docs/ folder.

[rijobro]

What's the use case for this? We expect input to be some permutation of [C, H, W, [D]]. What does it mean to have multiple channel dimensions?

[wyli]

What's the use case for this? We expect input to be some permutation of [C, H, W, [D]]. What does it mean to have multiple channel dimensions?

I'm refactoring the image writers and the nifti format and nibabel API assumes the spatial dimensions (up to 3d) first -- [spatial_dims, [time, modality]], and itk API supports [[vector], spatial_dims] (the 'spatial_dims' itself can be Nd). I feel a generic utility transform for moving these dimensions would be helpful.

[rijobro]

I agree it would be useful, though the name isn't valid if we're using it to move spatial dimensions (or other dimensions) around. Perhaps we should have a generic MoveAxis transform, and have AsChannelFirst etc call this?

[wyli]

I agree it would be useful, though the name isn't valid if we're using it to move spatial dimensions (or other dimensions) around. Perhaps we should have a generic MoveAxis transform, and have AsChannelFirst etc call this?

sure I see that keeping AsChannelFirst simple is good. Because there is a moveaxis utility, I feel adding MoveAxis is not needed... alternatively I can move this PR's logic to a as_channel_first(x, channel_dim=) in the transform utilities or close this PR, what do you think?

[Nic-Ma]

I agree it would be useful, though the name isn't valid if we're using it to move spatial dimensions (or other dimensions) around. Perhaps we should have a generic MoveAxis transform, and have AsChannelFirst etc call this?

sure I see that keeping AsChannelFirst simple is good. Because there is a moveaxis utility, I feel adding MoveAxis is not needed... alternatively I can move this PR's logic to a as_channel_first(x, channel_dim=) in the transform utilities or close this PR, what do you think?

Hi @wyli ,

Thanks for the explanation of the logic.
I feel "channel_dim is a sequence" is a little bit confusing and maybe the existing PR #3648 for moveaxis can satisfy the basic requirements? I would vote to keep this PR as draft or temporarily close it, let's revisit it later if see more clear use case and requirements when ImageWriter is ready.
What do you think?

Thanks.

[wyli]

Sure, thanks @rijobro @Nic-Ma, it's not a blocking issue, I keep it as a draft and will revisit it after the image writer work.

[ericspod]

@wyli do we want to push through this change still?

[wyli]

@wyli do we want to push through this change still?

sure, I'll rebase..

[wyli]

this transform is deprecated, if we still want this feature, it should be in EnsureChannelFirst cc @ericspod

[ericspod]

This was related to another task you were working on. If we haven't had the need to implement this I would close this PR and return the concept later, though it seems that Transpose has similar behaviour anyhow.