ENH: support additional dtypes in pad_nd #8672
Conversation
Prefer the PyTorch padding backend when supported and safely fall back to NumPy on error. Add unit tests to validate backend selection and ensure output dtype is preserved. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
shubham-61969
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
📝 WalkthroughWalkthroughpad_nd now prefers the PyTorch padding path for modes {"constant", "reflect", "edge", "replicate", "wrap", "circular"} irrespective of input dtype. It constructs a trimmed Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/transforms/croppad/functional.py (1)
99-110: Critical: NotImplementedError not caught by except clause.Line 103 catches
(ValueError, TypeError, RuntimeError)but line 104 checksisinstance(err, NotImplementedError). NotImplementedError would propagate uncaught, breaking the fallback mechanism. The test attest_pad_falls_back_to_np_if_pt_raisesexpects this fallback but would fail in real execution.🔎 Proposed fix
- except (ValueError, TypeError, RuntimeError) as err: + except (ValueError, TypeError, RuntimeError, NotImplementedError) as err: if isinstance(err, NotImplementedError) or any( k in str(err) for k in ("supported", "unexpected keyword", "implemented", "value") ):
🧹 Nitpick comments (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
49-58: Consider testing additional modes.Current tests only cover "constant" mode. The updated code supports {"reflect", "edge", "replicate", "wrap", "circular"} via PyTorch. Testing dtype preservation across these modes would strengthen coverage.
Optional enhancement
@pytest.mark.parametrize( "mode", ["constant", "reflect", "replicate"] ) @pytest.mark.parametrize( "dtype", [torch.bool, torch.int8, torch.float32] ) def test_pad_modes_with_dtypes(mode, dtype): """Test that pad_nd handles various modes and dtypes correctly.""" img = torch.ones((1, 4, 4), dtype=dtype) to_pad = [(0, 0), (1, 1), (2, 2)] out = pad_nd(img, to_pad, mode=mode, value=0) assert out.shape == (1, 6, 8) assert out.dtype == img.dtype
📜 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
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧰 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/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (2)
monai/transforms/croppad/functional.py (1)
monai/transforms/utils_pytorch_numpy_unification.py (1)
mode(426-438)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
monai/transforms/croppad/functional.py (3)
pad_nd(73-110)_pt_pad(59-70)_np_pad(45-56)
⏰ 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: flake8-py3 (pytype)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: packaging
- GitHub Check: min-dep-py3 (3.9)
Strip value for non-constant modes for both PyTorch and NumPy backends and ensure reliable fallback behavior. Update tests to cover multiple padding modes and dtype preservation. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/transforms/croppad/test_pad_nd_dtypes.py (4)
1-20: Module docstring and imports look good.Past review comments about missing module docstring have been addressed.
22-31: Test correctly validates PyTorch backend usage and dtype preservation.Past docstring feedback addressed.
33-45: Fallback test is well-structured.Mocking
_pt_padto raiseNotImplementedErrorand verifying_np_padis called covers the new fallback path. Past docstring feedback addressed.
47-57: Good dtype coverage.Tests bool, int8/16/32/64, uint8, float32. Past docstring feedback addressed.
🧹 Nitpick comments (2)
monai/transforms/croppad/functional.py (1)
99-112: Minor formatting: double spaces before**call_kwargs.Lines 107 and 112 have two spaces before
**call_kwargs.🔎 Fix spacing
except NotImplementedError: - return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) + return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) except (ValueError, TypeError, RuntimeError) as err: if any( k in str(err) for k in ("supported", "unexpected keyword", "implemented", "value") ): - return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs) + return _np_pad(img, pad_width=to_pad, mode=mode, **call_kwargs)tests/transforms/croppad/test_pad_nd_dtypes.py (1)
59-69: Consider omittingvalue=0for non-constant modes.Line 66 passes
value=0for all modes, but "reflect" and "replicate" don't use this parameter. While the implementation strips it, omitting it in tests makes the intent clearer.🔎 Suggested change
- out = pad_nd(img, to_pad, mode=mode, value=0) + kwargs = {"value": 0} if mode == "constant" else {} + out = pad_nd(img, to_pad, mode=mode, **kwargs)
📜 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
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧰 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/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (1)
monai/transforms/croppad/functional.py (1)
monai/transforms/utils_pytorch_numpy_unification.py (1)
mode(426-438)
⏰ 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: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: packaging
- GitHub Check: build-docs
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.8.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 (windows-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (macOS-latest)
🔇 Additional comments (1)
monai/transforms/croppad/functional.py (1)
102-105: Logic looks correct for strippingvaluein non-constant modes.Copying kwargs and removing "value" when mode != "constant" prevents passing unsupported arguments to reflect/replicate/etc. modes.
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/transforms/croppad/test_pad_nd_dtypes.py (4)
15-20: Import ordering could follow PEP8 more strictly.Group imports: standard library, third-party, local. Consider:
from __future__ import annotations + from unittest.mock import Mock, patch + import pytest import torch + import monai.transforms.croppad.functional as F from monai.transforms.croppad.functional import pad_nd
22-31: Consider verifying output shape and values.Test confirms backend selection and dtype preservation but doesn't check padding correctness. Add assertions:
assert out.shape == (1, 6, 8) assert out[0, 1, 2].item() == True # verify padded region if needed
33-45: Expand fallback testing to cover other exception types.The implementation also catches
ValueError,TypeError, andRuntimeErrorwith message matching. Test these paths:🔎 Additional test cases
@pytest.mark.parametrize("error_type,message", [ (ValueError, "not supported"), (TypeError, "unexpected keyword argument"), (RuntimeError, "not implemented"), ]) def test_pad_falls_back_on_other_errors(error_type, message): """Test fallback when PyTorch raises ValueError/TypeError/RuntimeError.""" img = torch.ones((1, 4, 4), dtype=torch.bool) to_pad = [(0, 0), (1, 1), (2, 2)] with ( patch.object(F, "_pt_pad", new=Mock(side_effect=error_type(message))), patch.object(F, "_np_pad", wraps=F._np_pad) as mock_np, ): out = pad_nd(img, to_pad, mode="constant", value=0) assert mock_np.called assert out.dtype == img.dtype
59-70: LGTM. Consider expanding mode coverage.Correctly handles
valuekwarg for constant mode only. For more comprehensive testing, add modes like"edge","wrap","circular".
📜 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
📒 Files selected for processing (2)
monai/transforms/croppad/functional.pytests/transforms/croppad/test_pad_nd_dtypes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/transforms/croppad/functional.py
🧰 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:
tests/transforms/croppad/test_pad_nd_dtypes.py
🧬 Code graph analysis (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
monai/transforms/croppad/functional.py (3)
pad_nd(73-119)_pt_pad(59-70)_np_pad(45-56)
⏰ 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: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
🔇 Additional comments (1)
tests/transforms/croppad/test_pad_nd_dtypes.py (1)
47-57: LGTM.Good dtype coverage. Shape and dtype assertions are appropriate for this test.
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @shubham-61969 thanks for the contribution. I think the change itself is fine with a few comments, the tests do need to be reformulated with unittest in particular. I think the previous implementation with its limitations was a result of older PyTorch versions so it's good to get this fix in. Please have a look again and then we can rereview.
| if mode != "constant": | ||
| call_kwargs.pop("value", None) |
There was a problem hiding this comment.
If this condition isn't present, the effect of providing a value argument and not use "constant" mode is to raise an exception in the pad routine used, I think this is intended behaviour. Here if the value argument is removed this silently allows unintended arguments to be ignored, it's better to raise an exception instead.
| @@ -0,0 +1,70 @@ | |||
| # Copyright (c) MONAI Consortium | |||
There was a problem hiding this comment.
In MONAI we use unittest and parameterized packages for tests and not pytest, specifically we use test classes and methods for unit tests rather than functions. Please reformulate these tests to use these packages according the style of other tests that are present here. It might make sense to add your tests to an existing file rather than a new one, have a look at existing files to see if it does make sense that way.
- Raise an explicit error when �alue is provided with non-constant modes. - Rewrite tests using unittest + parameterized to match MONAI style. Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
for more information, see https://pre-commit.ci
Signed-off-by: Shubham Chandravanshi <shubham.chandravanshi378@gmail.com>
…61969/MONAI into 7842-pad-nd-more-dtypes
|
Hi @ericspod , thanks for the feedback. I’ve updated the implementation to explicitly raise an error when value is provided with modes other than "constant", so the original contract is preserved. I also reworked the tests to use unittest + parameterized and aligned them with MONAI’s existing test style. All GitHub CI checks are now passing. Could you please take another look? If there are any further changes you’d like, I’m happy to address them otherwise I believe this should be ready to merge. |
2 participants
Prefer the PyTorch padding backend when supported and safely fall back
to NumPy on error. Add unit tests to validate backend selection and
ensure output dtype is preserved.
Fixes #7842
Description
This pull request relaxes dtype restrictions in
pad_ndand prefersthe PyTorch padding backend when supported, with a safe fallback to
NumPy on error. This enables support for additional dtypes (e.g. bool)
that are already handled correctly by recent PyTorch versions.
Unit tests are added to validate backend selection and ensure dtype
preservation.
Types of changes