MONAI icon indicating copy to clipboard operation
MONAI copied to clipboard
verified publisher icon Project-MONAI

Update Frequency Transform

Open ericspod opened this issue 5 months ago • 11 comments

Fixes #8520.

Description

This adds torchaudio as a dependency and changes SignalRemoveFrequency to explicitly convert input to double precision, this seems to be needed for new versions.

Types of changes

  • [x] 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.

ericspod avatar Aug 12 '25 01:08 ericspod

Walkthrough

  • monai/transforms/signal/array.py: SignalRemoveFrequency now converts the input signal to a PyTorch double tensor at the start, constructs iirnotch coefficients with dtype=torch.double, and calls filtfilt with the already-converted signal. Public API/signatures unchanged.
  • pyproject.toml, setup.cfg, requirements.txt: bumped minimum PyTorch from 2.4.1 to 2.5.1 (Windows constraint still excludes 2.7.0). NumPy constraints unchanged.
  • .github/workflows/pythonapp.yml: consolidated dependency installation into a single pip install command for torch==2.5.1, torchvision==0.20.1, and dev requirements; removed the separate torch/torchvision and -r requirements-dev steps.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Potential focus areas:

  • monai/transforms/signal/array.py: verify numeric dtype propagation, test behavior with float32 downstream code and multi-channel inputs.
  • Dependency bumps (pyproject.toml, setup.cfg, requirements.txt): confirm CI images and runtime environments support torch>=2.5.1 and that exclusion of torch==2.7.0 on Windows is intentional.
  • .github/workflows/pythonapp.yml: ensure caching, matrices, and environment setup still behave as expected after merging pip installs.
  • Tests related to linked issue #8520: confirm change resolves NumPy-related numeric test failures and does not introduce new precision regressions.

Pre-merge checks and finishing touches

❌ Failed checks (2 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive Title vaguely references 'Frequency Transform' but doesn't specify the actual fix: converting SignalRemoveFrequency to double precision and updating torch dependencies. Clarify title to indicate the actual changes, e.g., 'Fix SignalRemoveFrequency precision and update torch to 2.5.1' or similar.
Out of Scope Changes check ❓ Inconclusive Torch version bumps (2.4.1→2.5.1) across requirements, setup.cfg, pyproject.toml, and CI workflow align with fixing the test failure but lack explicit justification for version choice in PR description. Clarify why torch 2.5.1 specifically was chosen and confirm all dependency updates are necessary to resolve the NumPy 2.3.1 issue.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed Description covers the main changes (double precision conversion, torch update, torchaudio dependency) but lacks detail on why these changes fix the NumPy 2.3.1 compatibility issue.
Linked Issues check ✅ Passed PR addresses issue #8520 by adding torchaudio dependency and converting SignalRemoveFrequency to torch.double, matching the reported requirements for NumPy 2.3.1 compatibility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
📜 Recent review details

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 6a9c18e50ae23d7bbb9c0985a448e09a1040fcba and 7b78c53a42c2951935402ca45103a2714afdac5f.

📒 Files selected for processing (1)
  • setup.cfg (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • setup.cfg
⏰ 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: flake8-py3 (mypy)
  • GitHub Check: build-docs
  • GitHub Check: flake8-py3 (pytype)
  • GitHub Check: flake8-py3 (codeformat)
  • 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-pytorch (2.6.0)
  • GitHub Check: quick-py3 (ubuntu-latest)
  • GitHub Check: quick-py3 (windows-latest)
  • GitHub Check: min-dep-py3 (3.9)
  • GitHub Check: quick-py3 (macOS-latest)
  • GitHub Check: min-dep-py3 (3.10)
  • GitHub Check: packaging
  • GitHub Check: min-dep-py3 (3.11)
  • GitHub Check: min-dep-os (windows-latest)
  • GitHub Check: min-dep-os (macOS-latest)
  • GitHub Check: min-dep-os (ubuntu-latest)

[!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.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions: | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context. Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

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.

❤️ Share

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

coderabbitai[bot] avatar Aug 12 '25 01:08 coderabbitai[bot]

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.

ericspod avatar Aug 15 '25 02:08 ericspod

/build

KumoLiu avatar Aug 27 '25 08:08 KumoLiu

/build

KumoLiu avatar Sep 02 '25 14:09 KumoLiu

/build

KumoLiu avatar Sep 03 '25 15:09 KumoLiu

Seems several test run more than 1h, so the job was killed because it timed out. Not sure the reason.

[2025-09-03T16:29:02.711Z] Starting test: test_ahnet_shape_2d_0 (tests.networks.nets.test_ahnet.TestAHNET)...
[2025-09-03T17:12:41.972Z] Sending interrupt signal to process
[2025-09-03T17:12:42.124Z] Killing processes
[2025-09-03T17:12:43.462Z] kill finished with exit code 0
[2025-09-03T17:12:55.304Z] script returned exit code 143

[2025-09-03T15:46:46.615Z] Starting test: test_ahnet_shape_2d_0 (tests.networks.nets.test_ahnet.TestAHNET.test_ahnet_shape_2d_0)...
[2025-09-03T17:12:40.677Z] Sending interrupt signal to process
[2025-09-03T17:12:40.678Z] Killing processes
[2025-09-03T17:12:41.969Z] kill finished with exit code 0
[2025-09-03T17:12:48.583Z] Sending interrupt signal to process
[2025-09-03T17:12:48.584Z] Killing processes
[2025-09-03T17:12:49.897Z] kill finished with exit code 2
[2025-09-03T17:12:54.917Z] script returned exit code 143

[2025-09-03T15:30:15.183Z] .Finished test: test_script (tests.networks.nets.test_vnet.TestVNet) (1.33s)
[2025-09-03T15:30:15.184Z] Starting test: test_vnet_shape_0 (tests.networks.nets.test_vnet.TestVNet)...
[2025-09-03T17:12:30.927Z] Sending interrupt signal to process
[2025-09-03T17:12:30.927Z] Killing processes
[2025-09-03T17:12:32.260Z] kill finished with exit code 0
[2025-09-03T17:12:38.943Z] script returned exit code 143

KumoLiu avatar Sep 04 '25 07:09 KumoLiu

/build

KumoLiu avatar Sep 08 '25 10:09 KumoLiu

/build

KumoLiu avatar Sep 10 '25 06:09 KumoLiu

/build

KumoLiu avatar Sep 10 '25 07:09 KumoLiu

/build

KumoLiu avatar Sep 10 '25 15:09 KumoLiu

/build

KumoLiu avatar Sep 15 '25 07:09 KumoLiu