Update Frequency Transform #8537
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential focus areas:
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
🧹 Nitpick comments (1)
monai/transforms/signal/array.py (1)
409-409: Update function signature type hint.The function signature indicates it accepts
np.ndarraybut the implementation now works with any tensor-like input viaconvert_to_tensor. Consider updating the type hint to reflect actual usage.Apply this diff to update the type hint:
- def __call__(self, signal: np.ndarray) -> Any: + def __call__(self, signal: NdarrayOrTensor) -> torch.Tensor:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
monai/transforms/signal/array.py(1 hunks)requirements-dev.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.
Files:
monai/transforms/signal/array.py
🧬 Code Graph Analysis (1)
monai/transforms/signal/array.py (1)
monai/utils/type_conversion.py (1)
convert_to_tensor(113-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: packaging
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.10)
🔇 Additional comments (3)
monai/transforms/signal/array.py (3)
414-414: LGTM - Explicit double precision conversion addresses NumPy compatibility.Converting the signal to double precision upfront resolves the numerical precision issues reported in issue #8520 with NumPy 2.3.1.
415-417: LGTM - Consistent double precision for filter coefficients.The explicit dtype specification for iirnotch coefficients ensures numerical consistency with the input signal conversion.
418-418: LGTM - Simplified filtfilt call with pre-converted signal.The change eliminates redundant tensor conversion inside filtfilt since the signal is already converted to a tensor at line 414.
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
requirements.txt (1)
2-2: Justify and document the Windows-specific exclusion of torch 2.7.0.If 2.7.0 is known-bad only on Windows, add a short comment and ensure CI excludes it. If the issue isn’t Windows-only, consider mirroring the exclusion on non-Windows too.
Proposed inline comment:
torch>=2.5.1, !=2.7.0; platform_system == "Windows" +# Exclude torch 2.7.0 on Windows due to a known upstream issue; re-evaluate when fixed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
pyproject.toml(1 hunks)requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: packaging
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: build-docs
🔇 Additional comments (1)
requirements.txt (1)
1-1: Bump to torch>=2.5.1 (non-Windows) looks fine.Matches the pyproject change per PR notes. No syntax issues with the environment marker.
| torch>=2.5.1; platform_system != "Windows" | ||
| torch>=2.5.1, !=2.7.0; platform_system == "Windows" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm non-breaking claim and dependency consistency (torch, torchaudio, numpy).
- Raising the torch floor to 2.5.1 narrows supported environments. If the PR is marked “non-breaking,” confirm our supported matrix already requires ≥2.5.1 and update docs/release notes if needed.
- PR summary mentions “adds torchaudio as a dependency.” If torchaudio is only added to dev requirements, that’s fine; if runtime code now depends on it, add a runtime pin (or an extra like monai[audio]) and ensure torch/torchaudio version compatibility.
- Given the NumPy 2.3.1 test failure is the driver, ensure our NumPy pin and CI cover 2.x and that the dtype fix resolves the test across OSes.
Run this script to check for constraint consistency and runtime usage:
I can help propose extras (e.g., [audio]) and align pins once we see the scan output.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Torch constraints across repo =="
rg -n --no-heading -A2 -B2 $'torch\\s*[<>=!]' || true
echo
echo "== Torchaudio presence (constraints/imports) =="
rg -n --no-heading -A2 -B2 $'torchaudio' || true
echo
echo "== Numpy constraints (requirements*/pyproject) =="
rg -n --no-heading $'^numpy.*' requirements* pyproject.toml || true
echo
echo "== CI workflows referencing torch versions =="
fd -t f -a .github | xargs -I {} rg -n --no-heading -H 'torch|pytorch|TORCH_VERSION' {} || trueLength of output: 18092
Action required — align torch pins, confirm torchaudio runtime vs dev, and verify NumPy 2.x CI
Scan summary (concise): pyproject.toml and requirements.txt require torch>=2.5.1, but setup.cfg/docs/env still allow older torch/pytorch; torchaudio appears only in dev requirements; numpy is pinned >=1.24,<3.0.
Files needing attention:
- pyproject.toml — torch>=2.5.1 (OK if intended).
- requirements.txt — torch>=2.5.1 / Windows variant (OK).
- setup.cfg — install_requires has torch>=2.4.1 — update to >=2.5.1 to match or document the intended variance.
- environment-dev.yml — pytorch>=2.3.0 — update or justify (dev env still allows older PyTorch).
- docs/requirements.txt — references torch>=2.4.1 and a 2.4.1 CPU wheel — update docs wheel/pin to match new floor.
- requirements-dev.txt — contains torchaudio (dev only). If torchaudio is now a runtime dependency, move it to runtime deps (pyproject/setup.cfg/requirements) or provide an extra (e.g., [audio]) and pin to a torchaudio version compatible with torch>=2.5.1.
- monai/transforms/signal/array.py & tests/transforms/test_signal_remove_frequency.py — use optional_import/skipUnless for torchaudio; ensure CI either installs torchaudio for those tests or accepts skips.
- numpy (requirements.txt / setup.cfg / environment-dev.yml) — pinned >=1.24,<3.0. If NumPy 2.3.1 caused failures, add CI coverage for numpy==2.3.1 and confirm the dtype fix resolves the test across OSes.
- .github workflows — no references to torch/pytorch/TORCH_VERSION found; add/update CI matrix to explicitly exercise the declared minimal torch and numpy versions.
Please update the listed files or confirm the intended supported matrix and whether torchaudio is a runtime dependency; I can prepare concrete diffs if you want.
🤖 Prompt for AI Agents
In requirements.txt lines 1-2, align the torch pins across the repository:
update setup.cfg install_requires, environment-dev.yml, and
docs/requirements.txt to require torch>=2.5.1 (or document intentional
divergence), and remove the outdated 2.4.1 CPU wheel reference; decide whether
torchaudio is a runtime dependency—if it is, move it from requirements-dev.txt
into runtime deps (pyproject/setup.cfg/requirements) or expose it as an optional
extra (e.g., [audio]) with a torchaudio version compatible with torch>=2.5.1,
otherwise make all torchaudio uses in monai/transforms/signal/array.py and its
tests use optional_import/skipUnless so tests skip when torchaudio is absent;
pin numpy policy consistently (keep >=1.24,<3.0), add CI jobs to run with
numpy==2.3.1 and the minimal torch>=2.5.1 (including Windows variant), and
update GitHub workflow matrices to explicitly exercise the declared minimal
torch and numpy versions so CI validates the compatibility matrix.
…ent PyTorch version Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/pythonapp.yml (1)
95-98: Avoid double-installing torch/vision on Windows; make the general install step non-Windows or unify installs.Windows installs torch/vision via the PyTorch CPU index (Lines 95–98), then the general step reinstalls them (Line 107) without the index, risking re-resolution and mismatched wheels.
Option A (minimal): gate the general dependency step to non-Windows:
- - name: Install the dependencies + - if: runner.os != 'windows' + name: Install the dependencies run: | python -m pip install --user --upgrade pip wheel cat "requirements-dev.txt" - python -m pip install torch==2.5.1 torchvision==0.20.1 -r requirements-dev.txt + python -m pip install --index-url https://download.pytorch.org/whl/cpu torch==2.5.1 torchvision==0.20.1 torchaudio==2.5.1 + python -m pip install -r requirements-dev.txt python -m pip list python setup.py develop # test no compile installationOption B (preferred): remove the Windows-only torch step and rely on the unified CPU-index install for all OSes, to keep one source of truth. I can draft that change if you prefer.
Also applies to: 103-110
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
.github/workflows/pythonapp.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.11)
|
Hi @KumoLiu I think this works now. I had an issue with gloo not working under Windows with PyTorch 2.8 so dist tests wouldn't pass. I updated the CICD system to use the PyTorch 2.5.1 it should use for these tests in one place, but we should look into updating how the tests are run later to make this better. For now this fix works however, but other tests may be installing PyTorch 2.8 over top of version 2.5.1. |
|
/build |
|
/build |
|
/build |
|
Seems several test run more than 1h, so the job was killed because it timed out. Not sure the reason. |
|
/build |
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
setup.cfg (1)
111-112: Constrain torchaudio extra to avoid mismatched torch upgrades.Unbounded
torchaudiocan force pip to upgradetorchbeyond what CI targets. Add a floor (and consider mirroring fortorchvision).Apply:
-torchaudio = - torchaudio +torchaudio = + torchaudio>=2.5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
setup.cfg(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
🔇 Additional comments (1)
setup.cfg (1)
45-45: All torch constraints align on ≥2.5.1 (Windows also excludes 2.7.0). No cross-file inconsistencies detected.
setup.cfg
Outdated
| torchio | ||
| torchvision = | ||
| torchvision | ||
| torchaudio = |
There was a problem hiding this comment.
Can we remove this requirement? It will let the ci always downloading the latest pytorch.
There was a problem hiding this comment.
Ok I'll remove the requirement now and we'll try blossom again.
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
setup.cfg (1)
45-45: Alt: remove torch from install_requires to avoid resolver/CI churn.If CI/user envs preinstall torch, keeping it here can silently upgrade/downgrade to latest allowed wheels. Consider dropping it and documenting “install torch first”.
install_requires = - torch>=2.5.1 numpy>=1.24,<3.0Note: if you take this route, also consider whether
setup_requires = torchis still needed.
🧹 Nitpick comments (2)
setup.cfg (2)
49-61: If torchaudio is now a dependency, surface it (extras or core).PR text says torchaudio was added as a dependency; it’s not declared here. If it’s optional, add to extras (and to “all”); if it’s required at runtime, move to install_requires with appropriate markers.
all = nibabel ninja scikit-image>=0.14.2 scipy>=1.12.0; python_version >= '3.9' pillow tensorboard gdown>=4.7.3 pytorch-ignite==0.4.11 torchio torchvision + torchaudio itk>=5.2
109-111: Add a dedicated torchaudio extra for symmetry with torchvision.Keeps dependency management explicit without forcing heavy wheels on all users.
torchvision = torchvision +torchaudio = + torchaudio
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
setup.cfg(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: packaging
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (macOS-latest)
| install_requires = | ||
| torch>=2.4.1 | ||
| torch>=2.5.1 | ||
| numpy>=1.24,<3.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add Windows marker to exclude torch 2.7.0 here as well.
setup.cfg doesn’t reflect the Windows !=2.7.0 exclusion used elsewhere. Mirror it to avoid pulling the known-bad build on Windows.
install_requires =
- torch>=2.5.1
+ torch>=2.5.1; platform_system != "Windows"
+ torch>=2.5.1,!=2.7.0; platform_system == "Windows"
numpy>=1.24,<3.0🏁 Script executed:
#!/bin/bash
# search for torch references across the repo
rg -n "torch" -C2 .
# search for Windows exclusion of version 2.7.0
rg -n "!=2.7.0" -C2 .Length of output: 274858
Add Windows exclusion to setup.cfg install_requires
File setup.cfg lines 44–46: mirror requirements.txt to avoid torch 2.7.0 on Windows.
install_requires =
- torch>=2.5.1
+ torch>=2.5.1; platform_system != "Windows"
+ torch>=2.5.1,!=2.7.0; platform_system == "Windows"
numpy>=1.24,<3.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.
| install_requires = | |
| torch>=2.4.1 | |
| torch>=2.5.1 | |
| numpy>=1.24,<3.0 | |
| install_requires = | |
| torch>=2.5.1; platform_system != "Windows" | |
| torch>=2.5.1,!=2.7.0; platform_system == "Windows" | |
| numpy>=1.24,<3.0 |
🤖 Prompt for AI Agents
In setup.cfg around lines 44 to 46, the install_requires entry currently forces
torch for all platforms; update it to mirror requirements.txt and exclude
installing torch on Windows to avoid pulling torch 2.7.0. Change the torch
requirement to a platform-specific marker (e.g. "torch>=2.5.1; sys_platform !=
'win32'") or match whatever conditional used in requirements.txt, leaving numpy
unchanged, so Windows installs will skip torch and follow the mirrored
requirements.
|
/build |
1 similar comment
|
/build |
|
/build |
|
/build |
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
2 participants
Fixes #8520.
Description
This adds
torchaudioas a dependency and changesSignalRemoveFrequencyto explicitly convert input to double precision, this seems to be needed for new versions.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.