Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ITK Image to MONAI MetaTensor conversion #5897

Merged
merged 40 commits into from
Feb 20, 2023

Conversation

Shadow-Devil
Copy link
Contributor

@Shadow-Devil Shadow-Devil commented Jan 25, 2023

Fixes #5708
Fixes #4117

Description

This is a migration of the PR InsightSoftwareConsortium/itk-torch-transform-bridge#6 into MONAI.

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.

@Shadow-Devil Shadow-Devil changed the title Expose ITK Image to MONAI MetaTensor conversion [DRAFT] Expose ITK Image to MONAI MetaTensor conversion Jan 25, 2023
@Shadow-Devil Shadow-Devil changed the title [DRAFT] Expose ITK Image to MONAI MetaTensor conversion [WIP] Expose ITK Image to MONAI MetaTensor conversion Jan 25, 2023
@Spenhouet
Copy link
Contributor

Spenhouet commented Jan 27, 2023

tests/test_itk_torch_bridge.py Outdated Show resolved Hide resolved
@Shadow-Devil
Copy link
Contributor Author

Shadow-Devil commented Feb 4, 2023

@ntatsisk Thank you for fixing the issues described in the discussion of your PR! I've added your latest three commits, so InsightSoftwareConsortium/itk-torch-transform-bridge@12f4d4b InsightSoftwareConsortium/itk-torch-transform-bridge@37cc4f9 and InsightSoftwareConsortium/itk-torch-transform-bridge@7a253b2.
One problem I've found is that sometimes the test_random_array fails, maybe I should increase the tolerance?
And is there anything else that I should adjust?

@ntatsisk
Copy link

ntatsisk commented Feb 6, 2023

Thanks @Shadow-Devil! Yes, increasing the tolerance is one option. You could also copy-paste the output of a random array and keep it fixed, or another way is to set a value for the random seed. I am not aware what is the most common approach in the rest of the monai codebase.

@Shadow-Devil
Copy link
Contributor Author

Since most of this test uses torch.rand and np.random.rand I decided that setting the random seed of both torch and numpy would be best, or else we need to set specific img, spacing, direction and origin.
@wyli I don't know if this seed will interfere with any other tests or if it should be reset somehow. What do you think?

Another problem I saw within the pipeline is that a testcase is requesting too much memory. Should we skip this test in the quick run? How can I do that?

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
…ernal functions private

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
…o_metatensor and partly fix dtype warning

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
tests/test_itk_torch_bridge.py Outdated Show resolved Hide resolved
@Shadow-Devil
Copy link
Contributor Author

@ntatsisk Didn't you want both ITKImage -> Metatensor and Metatensor -> ITKImage conversions in this PR?
Currently there is only the function itk_image_to_metatensor. Will you also develop a metatensor_to_itk_image function in your PR?

@wyli
Copy link
Contributor

wyli commented Feb 9, 2023

@ntatsisk Didn't you want both ITKImage -> Metatensor and Metatensor -> ITKImage conversions in this PR?
Currently there is only the function itk_image_to_metatensor. Will you also develop a metatensor_to_itk_image function in your PR?

I think we can create the function, the implementation could be just a thin wrapper of this logic :

itk_obj = monai.data.ITKWriter.create_backend_obj(
    meta_tensor.array,
    channel_dim=None,
    affine=meta_tensor.affine,
    affine_lps_to_ras=False,  # False if the affine is in itk convention
)
# itk.imwrite(itk_obj, "output.nii.gz")

Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
Signed-off-by: Felix Schnabel <f.schnabel@tum.de>
@Shadow-Devil Shadow-Devil marked this pull request as ready for review February 20, 2023 00:28
@Shadow-Devil Shadow-Devil changed the title [WIP] Expose ITK Image to MONAI MetaTensor conversion Expose ITK Image to MONAI MetaTensor conversion Feb 20, 2023
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

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

thanks, it looks nice,
I'll fix a few style issues and merge it soon.

there is a warning message from this call np.dtype(itk.D):

in the future the `.dtype` attribute of a given datatype object must be a valid dtype instance. `data_type.dtype` may need to be coerced using `np.dtype(data_type.dtype)`. (Deprecated NumPy 1.20)

perhaps we should think about proper methods of getting equivalent dtypes like this

"get_numpy_dtype_from_string",
"get_torch_dtype_from_string",
"dtype_torch_to_numpy",
"dtype_numpy_to_torch",
"get_equivalent_dtype",
"convert_data_type",
"get_dtype",
"convert_to_cupy",
"convert_to_numpy",
"convert_to_tensor",
"convert_to_dst_type",

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@ntatsisk
Copy link

Hi @Shadow-Devil , @wyli. Thanks again for all this amazing work done here! I might visit again my two PRs to see if it is possible:

  • To make the affine bridge work for different array shapes between the image to be transformed and the reference image.
  • To add a reference image for DDF PR as well

What do you think is preferable: wait for a couple of weeks before merging, or merge now and append any changes later in monai?

@wyli
Copy link
Contributor

wyli commented Feb 20, 2023

thanks @ntatsisk, please feel free to create follow-up feature/pull requests... I think this PR is a fairly self-contained starting point.

also, perhaps we should have a meeting to discuss an overall design and roadmap, for example a possible direction is towards an object-oriented approach like InsightSoftwareConsortium/itk-torch-transform-bridge#6 (comment) please let me know @Shadow-Devil @ntatsisk if you are interested in this topic.

Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor

wyli commented Feb 20, 2023

/build

@Spenhouet
Copy link
Contributor

thanks @ntatsisk, please feel free to create follow-up feature/pull requests... I think this PR is a fairly self-contained starting point.

also, perhaps we should have a meeting to discuss an overall design and roadmap, for example a possible direction is towards an object-oriented approach like InsightSoftwareConsortium/itk-torch-transform-bridge#6 (comment) please let me know @Shadow-Devil @ntatsisk if you are interested in this topic.

If there is interest in this, I'm happy to participate and/or share updated code.

@wyli wyli merged commit 2a8c8cd into Project-MONAI:dev Feb 20, 2023
@ntatsisk
Copy link

Great to see this merged! It will be really valuable to us over (ITK)Elastix (InsightSoftwareConsortium/ITKElastix#126). Good job! 🎉

@wyli, Thanks for the reply, I might come back with more PRs then :). Related to your suggestion, I am always interested for further bridging opportunities, and a meeting would also be a nice opportunity to meet as well!

@dzenanz
Copy link
Contributor

dzenanz commented Feb 21, 2023

I, too, am glad to see this merged. It will make my life easier with #5711.

@Spenhouet
Copy link
Contributor

@ntatsisk Really looking forward to a solution with respect to different image sizes. That's the only bigger limitation I'm aware of right now.

@Shadow-Devil
Copy link
Contributor Author

also, perhaps we should have a meeting to discuss an overall design and roadmap, for example a possible direction is towards an object-oriented approach like InsightSoftwareConsortium/itk-torch-transform-bridge#6 (comment) please let me know @Shadow-Devil @ntatsisk if you are interested in this topic.

Sorry for the late response. I won't join this meeting, since this was my first encounter with ITK, so there isn't much I could contribute. I hope you will find a good design/roadmap for further development.
Thank you for merging this PR, I'm glad that we got a nice first version done.

@Shadow-Devil Shadow-Devil deleted the feature/ITK_bridge branch February 27, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants