Skip to content

Fft cleanup/update#8762

Merged
ericspod merged 3 commits intoProject-MONAI:devfrom
aymuos15:fft-cleanup
Mar 3, 2026
Merged

Fft cleanup/update#8762
ericspod merged 3 commits intoProject-MONAI:devfrom
aymuos15:fft-cleanup

Conversation

@aymuos15
Copy link
Contributor

@aymuos15 aymuos15 commented Mar 2, 2026

Remove legacy PyTorch 1.8.0 compatibility code from FFT utilities. MONAI now requires PyTorch ≥ 2.4.1, so the version checks and NumPy fallbacks are no longer needed.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).

aymuos15 added 2 commits March 2, 2026 05:28
Drop legacy version checks for fftshift/ifftshift and simplify fallback paths.
MONAI's minimum PyTorch version is now 2.4.1, making this code obsolete.

Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Remove notes about fallback behavior from fft_utils_t.py following removal
of PyTorch 1.8.0 compatibility code.

Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 2, 2026

📝 Walkthrough

Walkthrough

Removes embedded version check logic and related documentation from FFT utility functions. In fft_utils_t.py, removes docstring notes from four functions. In transforms/utils.py, eliminates runtime PyTorch version branching in Fourier.shift_fourier and Fourier.inv_shift_fourier, consolidating to direct use of torch.fft functions. Numeric computation paths unchanged for both Tensor and NumPy inputs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive Title refers to the main change (FFT cleanup) but is somewhat vague and generic. Consider a more specific title like 'Remove legacy PyTorch 1.8.0 compatibility from FFT utilities' to clarify what cleanup entails.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed Description includes the rationale and change type, but lacks detail on what specific version checks and fallback paths were removed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
monai/transforms/utils.py (1)

1886-1908: ⚠️ Potential issue | 🟡 Minor

Remove unused parameter n_dims.

The n_dims parameter is not referenced in the function body and is not documented in the Args section. Callers pass values to the spatial_dims parameter (second positional argument), leaving n_dims unused throughout.

Proposed fix
     `@staticmethod`
     def inv_shift_fourier(
-        k: NdarrayOrTensor, spatial_dims: int, n_dims: int | None = None, as_contiguous: bool = False
+        k: NdarrayOrTensor, spatial_dims: int, as_contiguous: bool = False
     ) -> NdarrayOrTensor:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monai/transforms/utils.py` around lines 1886 - 1908, The function
inv_shift_fourier has an unused parameter n_dims; remove n_dims from the
function signature and any internal references, update the docstring Args to
drop the n_dims entry, and adjust any call sites that currently pass a third
argument to pass only spatial_dims (or use keyword as_contiguous) so callers
match the new signature; keep the logic using spatial_dims and as_contiguous
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@monai/transforms/utils.py`:
- Around line 1886-1908: The function inv_shift_fourier has an unused parameter
n_dims; remove n_dims from the function signature and any internal references,
update the docstring Args to drop the n_dims entry, and adjust any call sites
that currently pass a third argument to pass only spatial_dims (or use keyword
as_contiguous) so callers match the new signature; keep the logic using
spatial_dims and as_contiguous unchanged.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 894068a and 1a1156c.

📒 Files selected for processing (2)
  • monai/networks/blocks/fft_utils_t.py
  • monai/transforms/utils.py
💤 Files with no reviewable changes (1)
  • monai/networks/blocks/fft_utils_t.py

@aymuos15
Copy link
Contributor Author

aymuos15 commented Mar 2, 2026

I guess the coderabbit comment is preexisting and not in the scope of this PR? I can make that change separately after the decision on this change?

Copy link
Member

@ericspod ericspod 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 cleanup!

@ericspod
Copy link
Member

ericspod commented Mar 3, 2026

I guess the coderabbit comment is preexisting and not in the scope of this PR? I can make that change separately after the decision on this change?

It appears so, yes. It should be removed with a deprecation warning which can be done as a separate PR, though it seems to be low priority.

@ericspod ericspod enabled auto-merge (squash) March 3, 2026 17:11
@aymuos15
Copy link
Contributor Author

aymuos15 commented Mar 3, 2026

I guess the coderabbit comment is preexisting and not in the scope of this PR? I can make that change separately after the decision on this change?

It appears so, yes. It should be removed with a deprecation warning which can be done as a separate PR, though it seems to be low priority.

Sounds good. Thank you very much. Ill make one then after this is merged. I haven't looked into it to really verify what all that bit of code is touching.

@ericspod ericspod merged commit 9ddd5e6 into Project-MONAI:dev Mar 3, 2026
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.

2 participants