Remove unused n_dims parameter from Fourier.inv_shift_fourier#8766
Remove unused n_dims parameter from Fourier.inv_shift_fourier#8766aymuos15 wants to merge 2 commits intoProject-MONAI:devfrom
n_dims parameter from Fourier.inv_shift_fourier#8766Conversation
📝 WalkthroughWalkthroughThe Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/transforms/utils.py`:
- Around line 1887-1889: The public kwarg n_dims was removed from
inv_shift_fourier which will break callers using n_dims=; restore a deprecated
no-op parameter by adding n_dims: Optional[int] = None to the inv_shift_fourier
signature, emit a warnings.warn(..., DeprecationWarning) if n_dims is not None,
and ignore its value when computing (i.e., retain current behavior driven by
spatial_dims and as_contiguous). Update the function signature and add the
one-line deprecation warning near the top of inv_shift_fourier so downstream
code continues to work for one release cycle.
ℹ️ 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
📒 Files selected for processing (1)
monai/transforms/utils.py
monai/transforms/utils.py
Outdated
| 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: |
There was a problem hiding this comment.
Add a deprecation bridge for the removed public n_dims argument.
Line 1888 removes a public kwarg; downstream calls using n_dims= will now fail with TypeError. Please keep a deprecated no-op arg for one release cycle (or gate this behind a clearly documented major-version break).
Proposed compatibility patch
`@staticmethod`
def inv_shift_fourier(
- k: NdarrayOrTensor, spatial_dims: int, as_contiguous: bool = False
+ k: NdarrayOrTensor,
+ spatial_dims: int,
+ n_dims: int | None = None,
+ as_contiguous: bool = False,
) -> NdarrayOrTensor:
@@
+ if n_dims is not None:
+ warnings.warn(
+ "`n_dims` is deprecated and ignored; use `spatial_dims` only.",
+ FutureWarning,
+ stacklevel=2,
+ )
dims = tuple(range(-spatial_dims, 0))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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: | |
| `@staticmethod` | |
| def inv_shift_fourier( | |
| k: NdarrayOrTensor, | |
| spatial_dims: int, | |
| n_dims: int | None = None, | |
| as_contiguous: bool = False, | |
| ) -> NdarrayOrTensor: | |
| if n_dims is not None: | |
| warnings.warn( | |
| "`n_dims` is deprecated and ignored; use `spatial_dims` only.", | |
| FutureWarning, | |
| stacklevel=2, | |
| ) | |
| dims = tuple(range(-spatial_dims, 0)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/transforms/utils.py` around lines 1887 - 1889, The public kwarg n_dims
was removed from inv_shift_fourier which will break callers using n_dims=;
restore a deprecated no-op parameter by adding n_dims: Optional[int] = None to
the inv_shift_fourier signature, emit a warnings.warn(..., DeprecationWarning)
if n_dims is not None, and ignore its value when computing (i.e., retain current
behavior driven by spatial_dims and as_contiguous). Update the function
signature and add the one-line deprecation warning near the top of
inv_shift_fourier so downstream code continues to work for one release cycle.
There was a problem hiding this comment.
This has been unused for a very long time. I doubt anyone will crash because of this.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all calls to inv_shift_fourier in the codebase
echo "=== Searching for inv_shift_fourier usage ==="
rg -n -C3 'inv_shift_fourier' --type=py
echo ""
echo "=== Checking if n_dims parameter is ever passed ==="
rg -n 'inv_shift_fourier.*n_dims' --type=py
echo ""
echo "=== Searching in tests specifically ==="
fd -e py -p '*test*.py' --exec rg -l 'inv_shift_fourier' {}
Unrecoverable error: Failed to clone repository
@aymuos15, understood. If the parameter has been unused for that long, the breaking change risk is minimal. Your judgment on this makes sense given your familiarity with the codebase and the prior discussion in #8762.
aymuos15
force-pushed
the
remove-unused-n_dims
branch
from
240e83a to
9303468
Compare
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
remove-unused-n_dims
branch
from
18c38a2 to
25d47e4
Compare
There was a problem hiding this comment.
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)
1887-1906:⚠️ Potential issue | 🟡 MinorAdd coverage for the signature change.
inv_shift_fourieris a modified public definition, but no related test update is shown here. Please add/update tests for the new call form and expected behavior whenn_dimsis passed.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/utils.py` around lines 1887 - 1906, Update unit tests to cover the modified public signature of inv_shift_fourier: add tests that call monai.transforms.utils.inv_shift_fourier with the new keyword form n_dims (in addition to existing spatial_dims calls) to ensure both produce identical outputs; include cases for both torch.Tensor and numpy.ndarray inputs, assert correct real-valued output after inverse shift+ifft, and verify as_contiguous=True returns a contiguous array/tensor while False preserves original contiguity. Target tests around the inv_shift_fourier function, using small 1D/2D/3D k-space examples and compare results between calls using spatial_dims=<n> and n_dims=<n> to validate the signature change is covered.
🤖 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 1887-1906: Update unit tests to cover the modified public
signature of inv_shift_fourier: add tests that call
monai.transforms.utils.inv_shift_fourier with the new keyword form n_dims (in
addition to existing spatial_dims calls) to ensure both produce identical
outputs; include cases for both torch.Tensor and numpy.ndarray inputs, assert
correct real-valued output after inverse shift+ifft, and verify
as_contiguous=True returns a contiguous array/tensor while False preserves
original contiguity. Target tests around the inv_shift_fourier function, using
small 1D/2D/3D k-space examples and compare results between calls using
spatial_dims=<n> and n_dims=<n> to validate the signature change is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07cec335-235f-41e7-b958-fe216a1bd486
📒 Files selected for processing (1)
monai/transforms/utils.py
1 participant
As discussed here: #8762