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

Forced Fourier class to output contiguous tensors. #7969

Merged
merged 5 commits into from
Sep 3, 2024

Conversation

bwittmann
Copy link
Contributor

Forced Fourier class to output contiguous tensors, which potentially fixes a performance bottleneck.

Description

Some transforms, such as RandKSpaceSpikeNoise, rely on the Fourier class.
In its current state, the Fourier class returns non-contiguous tensors, which potentially limits performance.
For example, when followed by RandHistogramShift, the following warning occurs:

<path_to_monai>/monai/transforms/intensity/array.py:1852: UserWarning: torch.searchsorted(): input value tensor is non-contiguous, this will lower the performance due to extra data copy when converting non-contiguous tensor to contiguous, please use contiguous input value tensor if possible. This message will only appear once per program. (Triggered internally at /opt/conda/conda-bld/pytorch_1716905975447/work/aten/src/ATen/native/BucketizationUtils.h:32.)
  indices = ns.searchsorted(xp.reshape(-1), x.reshape(-1)) - 1

A straightforward fix is to force the Fourier class to output contiguous tensors (see commit).
To reproduce, please run:

from monai.transforms import RandKSpaceSpikeNoise
from monai.transforms.utils import Fourier
import numpy as np

### TEST WITH TRANSFORMS ###   
t = RandKSpaceSpikeNoise(prob=1)

# for torch tensors
a_torch = torch.rand(1, 128, 128, 128)
print(a_torch.is_contiguous())

a_torch_mod = t(a_torch)
print(a_torch_mod.is_contiguous())

# for np arrays
a_np = np.random.rand(1, 128, 128, 128)
print(a_np.flags['C_CONTIGUOUS'])

a_np_mod = t(a_np)  # automatically transformed to torch.tensor
print(a_np_mod.is_contiguous())

### TEST DIRECTLY WITH FOURIER ###
f = Fourier()

# inv_shift_fourier
# for torch tensors
real_torch = torch.randn(1, 128, 128, 128)
im_torch = torch.randn(1, 128, 128, 128)
k_torch = torch.complex(real_torch, im_torch)
print(k_torch.is_contiguous())

out_torch = f.inv_shift_fourier(k_torch, spatial_dims=3)
print(out_torch.is_contiguous())

# for np arrays
real_np = np.random.randn(1, 100, 100, 100)
im_np = np.random.randn(1, 100, 100, 100)
k_np = real_np + 1j * im_np
print(k_np.flags['C_CONTIGUOUS'])

out_np = f.inv_shift_fourier(k_np, spatial_dims=3)
print(out_np.flags['C_CONTIGUOUS'])

# shift_fourier
# for torch tensors
a_torch = torch.rand(1, 128, 128, 128)
print(a_torch.is_contiguous())

out_torch = f.shift_fourier(a_torch, spatial_dims=3)
print(out_torch.is_contiguous())

# for np arrays
a_np = np.random.rand(1, 128, 128, 128)
print(a_np.flags['C_CONTIGUOUS'])

out_np = f.shift_fourier(a_np, spatial_dims=3)
print(out_np.flags['C_CONTIGUOUS'])

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.

@bwittmann bwittmann changed the title Forced Fourier class to output contiguous() tensors. Forced Fourier class to output contiguous tensors. Jul 31, 2024
monai/transforms/utils.py Outdated Show resolved Hide resolved
@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 26, 2024

Hi @bwittmann, do you plan to finish this PR? If we want to include the change in version 1.4, we only have about one or two weeks left to add new things.

@bwittmann
Copy link
Contributor Author

Dear @KumoLiu,

following your recommendation, I added as_contiguous as an argument to propose an experimental option.

Best,
Bastian

Copy link
Contributor

@KumoLiu KumoLiu 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 update, LGTM.

@KumoLiu
Copy link
Contributor

KumoLiu commented Sep 3, 2024

Hi @bwittmann, please help fix the DCO and format issue. Thanks.

DCO Remediation Commit for Bastian Wittmann <bastian.wittmann@uzh.ch>

I, Bastian Wittmann <bastian.wittmann@uzh.ch>, hereby add my Signed-off-by to this commit: fb65555
I, Bastian Wittmann <bastian.wittmann@uzh.ch>, hereby add my Signed-off-by to this commit: 4df5947

Signed-off-by: Bastian Wittmann <bastian.wittmann@uzh.ch>.
@KumoLiu
Copy link
Contributor

KumoLiu commented Sep 3, 2024

You can refer to the instruction here to fix the DCO issue.
https://github.com/Project-MONAI/MONAI/pull/7969/checks?check_run_id=29602735375

KumoLiu and others added 2 commits September 3, 2024 18:09
I, Bastian Wittmann <bastian.wittmann@uzh.ch>, hereby add my Signed-off-by to this commit: 24c195a

Signed-off-by: Bastian Wittmann <bastian.wittmann@uzh.ch>
@KumoLiu
Copy link
Contributor

KumoLiu commented Sep 3, 2024

/build

@KumoLiu KumoLiu enabled auto-merge (squash) September 3, 2024 12:34
@KumoLiu KumoLiu merged commit befb5f6 into Project-MONAI:dev Sep 3, 2024
28 checks passed
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.

3 participants