Skip to content

Fix: correctly apply fftshift to real-valued data inputs #8407

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

Merged
merged 10 commits into from
Apr 7, 2025

Conversation

XwK-P
Copy link
Contributor

@XwK-P XwK-P commented Mar 31, 2025

Correctly apply fftshift to real-valued data inputs

Fixes # .

Description

Correctly apply fftshift to real-valued data inputs. The last dimension of output of torch.view_as_real has size 2 for both real-valued and complex-valued input.

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.

XwK-P added 4 commits March 31, 2025 12:44
Correctly apply fftshift to real-valued data inputs

Signed-off-by: Puyang Wang <[email protected]>
Signed-off-by: Puyang Wang <[email protected]>
Signed-off-by: Puyang Wang <[email protected]>
@KumoLiu KumoLiu requested a review from Copilot April 1, 2025 03:12
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request fixes the application of fftshift for real-valued data inputs by adjusting how dimension indices are shifted when using torch.view_as_real.

  • The expected output in the FFT shift test has been updated to match the corrected behavior.
  • In the fft_utils_t module, the logic to compute shift indices has been modified for complex-valued inputs, ensuring consistency between ifftshift and fftshift.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/data/test_fft_utils.py Updates expected test output to reflect the new fftshift behavior.
monai/networks/blocks/fft_utils_t.py Refactors shift index calculation using a transformation on dims for fftshift and ifftshift.
Comments suppressed due to low confidence (3)

monai/networks/blocks/fft_utils_t.py:194

  • [nitpick] The expression '[d - 1 for d in dims]' is used to adjust the dimension indices for FFT operations; consider extracting it into a well-named variable or function to improve readability and clarify its intent.
x = ifftshift(im, [d - 1 for d in dims])

monai/networks/blocks/fft_utils_t.py:203

  • [nitpick] The same '[d - 1 for d in dims]' transformation is used here; consider refactoring it to avoid repetition and enhance clarity on why the indices are shifted by one.
out: Tensor = fftshift(x, [d - 1 for d in dims])

tests/data/test_fft_utils.py:24

  • Verify that the updated expected output aligns with the corrected fftshift behavior for both real-valued and complex-valued inputs, ensuring full coverage of the new logic.
[[[0.0, 0.0], [0.0, 0.0], [0.0, 0.0]], [[0.0, 0.0], [3.0, 0.0], [0.0, 0.0]], [[0.0, 0.0], [0.0, 0.0], [0.0, 0.0]]]

XwK-P added 3 commits April 1, 2025 15:36
Signed-off-by: Puyang Wang <[email protected]>
Signed-off-by: Puyang Wang <[email protected]>
@tangy5
Copy link
Contributor

tangy5 commented Apr 1, 2025

Thanks for the contribution, Puyang. @XwK-P

XwK-P added 2 commits April 1, 2025 17:42
Signed-off-by: Puyang Wang <[email protected]>
Signed-off-by: Puyang Wang <[email protected]>
@ericspod ericspod requested review from Nic-Ma, KumoLiu and tangy5 April 4, 2025 13:59
@ericspod
Copy link
Member

ericspod commented Apr 4, 2025

Hi @tangy5, would you be willing to fully review and approve is nothing is needed? I don't use these functions so it's hard to comment on correctness. Thanks!

@KumoLiu
Copy link
Contributor

KumoLiu commented Apr 7, 2025

/build

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.
@tangy5, let me know if you have any concern. Thanks.

@KumoLiu KumoLiu merged commit 83dcd35 into Project-MONAI:dev Apr 7, 2025
26 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.

4 participants