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

RandGaussianNoise and SavitzkyGolaySmooth Torch/NumpyTransforms #2740

Merged
merged 24 commits into from
Aug 17, 2021

Conversation

rijobro
Copy link
Contributor

@rijobro rijobro commented Aug 10, 2021

Related #2231, #2278.

Description

This is the first of many PRs to add support for Torch and Numpy input in our transforms, minimising conversion between the two and allowing for GPU data.

In this PR I've added the basic components:

  • One example of a transform that is ambivalent to Torch/Numpy input (RandGaussianSmooth)
  • One example of a transform that potentially requires conversion (SavitzkyGolaySmooth)
  • Functionality for switching between types
  • Functionality for harmonising dtypes
  • Tests use numpy, torch and torch.gpu input data
  • Convenience enums (DataObjects)

Status

Ready/Work in progress/Hold

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • 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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro rijobro requested review from wyli, ericspod and Nic-Ma August 10, 2021 11:10
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@wyli
Copy link
Contributor

wyli commented Aug 11, 2021

thanks for the PR, I'm looking into the details. @Nic-Ma could you please help review it as well?

@rijobro
Copy link
Contributor Author

rijobro commented Aug 11, 2021

@wyli The test seems to be failing on GaussianFilter, but that hasn't been modified. Any ideas? I tried re-running the tests.

@Nic-Ma
Copy link
Contributor

Nic-Ma commented Aug 11, 2021

Hi @rijobro @wyli ,

Thanks for the quick PR, I planned to review it today, but I am blocked by other internal task now.
Will try to review it ASAP.

Thanks.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
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.

looks good to me, I mainly have some comments on the styles and type hints for discussions @Project-MONAI/core-reviewers (and testing the github team feature...)

monai/utils/misc.py Outdated Show resolved Hide resolved
monai/utils/misc.py Outdated Show resolved Hide resolved
monai/utils/misc.py Outdated Show resolved Hide resolved
monai/transforms/intensity/array.py Outdated Show resolved Hide resolved
monai/utils/misc.py Outdated Show resolved Hide resolved
monai/utils/misc.py Outdated Show resolved Hide resolved
monai/transforms/transform.py Outdated Show resolved Hide resolved
@wyli
Copy link
Contributor

wyli commented Aug 12, 2021

@wyli The test seems to be failing on GaussianFilter, but that hasn't been modified. Any ideas? I tried re-running the tests.

looks like it's been addressed by rerunning, probably a GPU testing job scheduling issue, we'll migrate to blossom soon

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@madil90
Copy link
Contributor

madil90 commented Aug 12, 2021

/build

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Sorry for the delay response, very happy to see the great progress of this task.
The overall design looks good to me, put some minor comments inline.
And I want to double confirm the direction: do we want to change transforms to accept both numpy and tensor data and use PyTorch tensor for the real computation logic to support differentiable transforms later?

Thanks.

monai/transforms/intensity/dictionary.py Outdated Show resolved Hide resolved
monai/utils/misc.py Outdated Show resolved Hide resolved
monai/utils/type_conversion.py Outdated Show resolved Hide resolved
monai/utils/type_conversion.py Outdated Show resolved Hide resolved
monai/utils/type_conversion.py Outdated Show resolved Hide resolved
monai/utils/enums.py Outdated Show resolved Hide resolved
monai/transforms/intensity/array.py Outdated Show resolved Hide resolved
monai/transforms/transform.py Show resolved Hide resolved
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Aug 13, 2021

@Nic-Ma

do we want to change transforms to accept both numpy and tensor data and use PyTorch tensor for the real computation logic to support differentiable transforms later?

Correct, the majority of transforms won't need numpy<->torch conversion, so adding in differentiable transforms should be possible.

@rijobro
Copy link
Contributor Author

rijobro commented Aug 13, 2021

@wyli some flake8 errors have appeared since switching to Ndarraytensor: https://github.com/Project-MONAI/MONAI/pull/2740/checks?check_run_id=3322770593#step:7:376.

In this snippet:

   def __call__(self, img: NdarrayTensor) -> NdarrayTensor:
        """
        Apply the transform to `img`.
        """
        self.randomize(img.shape)
        if self._noise is None:
            raise AssertionError
        if not self._do_transform:
            return img
        noise, *_ = convert_data_type(
            self._noise, type(img), dtype=img.dtype, device=img.device if isinstance(img, torch.Tensor) else None
        )
        return img + noise

It's complaining about img.device for np.ndarray. As in, it thinks img is only of type np.ndarray, even though it's of type NdarrayTensor, which should be both np.ndarray and torch.Tensor.

Copy link
Contributor

@Nic-Ma Nic-Ma left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update.
Looks good to me now.

Thanks.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Aug 16, 2021

@wyli I'm still struggling with the flake8 problem, do you think you could take a look? Cheers

@wyli
Copy link
Contributor

wyli commented Aug 16, 2021

Sure I can look into this later today

rijobro and others added 3 commits August 16, 2021 11:29
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the torch_transforms branch from 7744319 to bc4c98f Compare August 16, 2021 18:26
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the torch_transforms branch from 73e5c90 to 108942b Compare August 16, 2021 20:00
wyli added 2 commits August 17, 2021 08:33
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli wyli force-pushed the torch_transforms branch from fd61de5 to b680c19 Compare August 17, 2021 08:01
wyli and others added 3 commits August 17, 2021 09:42
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Signed-off-by: Nic Ma <nma@nvidia.com>
@wyli wyli force-pushed the torch_transforms branch from 587cbfd to d41ed96 Compare August 17, 2021 08:42
@wyli wyli enabled auto-merge (squash) August 17, 2021 08:52
@wyli wyli disabled auto-merge August 17, 2021 16:13
@wyli wyli enabled auto-merge (squash) August 17, 2021 16:14
@wyli wyli disabled auto-merge August 17, 2021 16:15
@wyli wyli merged commit cf6edc7 into Project-MONAI:dev Aug 17, 2021
@rijobro rijobro deleted the torch_transforms branch August 18, 2021 13:22
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.

5 participants