Skip to content

Add to_monai and from_monai methods - #454

Open
mccle wants to merge 14 commits into
v0.29.0devfrom
feature/monai_volume_conversion
Open

mccle wants to merge 14 commits into
v0.29.0devfrom
feature/monai_volume_conversion

Conversation

@mccle

@mccle mccle commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@mccle
mccle changed the base branch from master to v0.29.0dev August 6, 2026 19:08
@mccle
mccle marked this pull request as ready for review September 3, 2026 19:25
@mccle
mccle requested a review from CPBridge September 3, 2026 20:02
Comment thread src/highdicom/volume.py Outdated
Comment thread src/highdicom/volume.py Outdated
)

metatensor = monai.data.MetaTensor(
self.array.copy(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this copy is necessary? I imagine the conversion to metatensor copies anyway and the memcopy could be quite slow, so best to avoid it if possible

Comment thread src/highdicom/volume.py Outdated
Comment thread src/highdicom/volume.py
Comment on lines +4315 to +4316
if ensure_channel_first:
metatensor = monai.transforms.EnsureChannelFirst()(metatensor)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a channel dim in the Volume and ensure_channel_first is False, the channel is placed last in the metatensor. I'm not sure this makes sense since torch never uses this format. I think it would be more expected behavior to move it first

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be consistent with the default behavior that MONAI uses for data loading. I saved a 4D NIfTI with NiBabel since it is also channel last and the LoadImage transform also produces a channel last metatensor without specifying ensure_channel_first=True.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a more sensible default in my opinion, but I think it should follow the behavior of the corresponding MONAI features.

Comment thread src/highdicom/volume.py
Comment on lines +4244 to +4248
def to_monai(
self,
convert_to_ras: bool = True,
ensure_channel_first: bool = False,
) -> 'monai.data.MetaTensor': # noqa: F821

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably add an option to squeeze the first spatial dimension (usually the slice dimension) if it is singleton

Comment thread src/highdicom/volume.py Outdated
Comment on lines +4395 to +4399
if array.ndim > 4:
raise ValueError(
'Monai conversion does not currently support'
' volumes with multiple channel dimensions.'
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test could be moved before the detach and numpy conversion

Comment thread tests/test_monai.py
)


try:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to test for ITK too, since you are using the ITKReader?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are currently run with none of the optional dependencies or with all of them, so there should not be a situation where the test is run and monai is installed but itk is not.

Comment thread tests/test_monai.py
@@ -0,0 +1,695 @@
import numpy as np

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably add a test with ensure_channels_first=True for both a volume with and without a channel dim to check it deals with them correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants