Fix box_iou returning 0 for floating-point results less than 1. #8369 #8553
Conversation
reworld223
requested review from
KumoLiu,
Nic-Ma and
ericspod
as code owners
WalkthroughDtype handling in monai/data/box_utils.py was changed for box_iou, box_giou, and box_pair_giou so outputs are always floating-type containers (NumPy ndarray or Torch tensor) with shapes (N, M) or (N,). If the first input is floating, its dtype is preserved; if integer, results use torch.float32. Intermediate computations use a fixed compute dtype, results are cast back to the target dtype before converting to the destination container, NaN/Inf checks remain, and convert_to_dst_type calls now explicitly include dtype=box_dtype. Public APIs are unchanged. Tests were added (tests/data/test_box_utils.py) including a TestBoxUtilsDtype class with two tests and an added torch import. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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)
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: 0
🧹 Nitpick comments (2)
monai/data/box_utils.py (2)
829-831: Docstring nit: tighten wording and dtype spelling.Minor clarity tweak: hyphenate “floating-point” and spell out torch dtype.
- The output is always floating point: - - if ``boxes1`` has a floating point dtype, the same dtype is used. - - if ``boxes1`` has an integer dtype, the result is returned as ``float32``. + The output is always floating-point: + - if ``boxes1`` has a floating-point dtype, the same dtype is used. + - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``.
847-852: Optional: apply same “always-float output” policy to GIoU helpers for consistency.box_giou/box_pair_giou currently cast to boxes1’s dtype, which can be integer; consider mirroring box_iou’s behavior.
Example patch for box_giou:
@@ - box_dtype = boxes1_t.dtype + box_dtype = boxes1_t.dtype @@ - giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) - giou_t = giou_t.to(dtype=box_dtype) + giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) + result_dtype = box_dtype if box_dtype.is_floating_point else COMPUTE_DTYPE + giou_t = giou_t.to(dtype=result_dtype) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=result_dtype)And similarly for box_pair_giou:
@@ - giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore - giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) + giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore + result_dtype = box_dtype if box_dtype.is_floating_point else COMPUTE_DTYPE + giou_t = giou_t.to(dtype=result_dtype) # (N,spatial_dims) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=result_dtype)
📜 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)
monai/data/box_utils.py(2 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/data/box_utils.py
⏰ 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-pytorch (2.5.1)
- 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-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: quick-py3 (macOS-latest)
🔇 Additional comments (2)
monai/data/box_utils.py (2)
847-852: Fix correctly avoids int truncation in IoU.Casting to the boxes1 float dtype (or torch.float32 for int inputs) after float32 compute resolves the 0-IoU bug. Conversion via convert_to_dst_type with dtype explicitly set is solid.
Also applies to: 858-858
847-852: Add tests to lock in dtype behavior.Please add unit tests covering torch and numpy paths to prevent regressions.
Suggested tests:
# tests/test_box_iou_dtype.py import numpy as np import torch from monai.data.box_utils import box_iou def _boxes(): # two 2D boxes with 50% overlap b1 = np.array([[0, 0, 2, 2]], dtype=np.int64) b2 = np.array([[1, 1, 3, 3]], dtype=np.int64) return b1, b2 def test_box_iou_numpy_int_returns_float32(): b1, b2 = _boxes() out = box_iou(b1, b2) assert out.dtype == np.float32 assert 0 < out[0, 0] < 1 def test_box_iou_torch_int_returns_float32(): b1, b2 = _boxes() out = box_iou(torch.as_tensor(b1), torch.as_tensor(b2)) assert out.dtype == torch.float32 assert 0 < out[0, 0].item() < 1 def test_box_iou_preserves_float64_when_input_is_float64(): b1, b2 = _boxes() out = box_iou(torch.as_tensor(b1, dtype=torch.float64), torch.as_tensor(b2, dtype=torch.float64)) assert out.dtype == torch.float64 assert 0 < out[0, 0].item() < 1
Signed-off-by: reworld223 <[email protected]>
Signed-off-by: reworld223 <[email protected]>
Signed-off-by: reworld223 <[email protected]>
reworld223
force-pushed
the
8369-ci-pipeline-setup
branch
from
9832ac2 to
bc99283
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
monai/data/box_utils.py (5)
873-875: Align GIoU return dtype semantics with IoU.GIoU still documents “same data type as boxes1”, which will truncate when boxes1 is int. Recommend matching IoU’s rule (float output; preserve boxes1 floating dtype, else float32).
Apply:
- Returns: - GIoU, with size of (N,M) and same data type as ``boxes1`` + Returns: + GIoU with size (N, M). Output is always floating point: + - if ``boxes1`` has a floating dtype, the same dtype is used; + - if ``boxes1`` has an integer dtype, ``float32`` is used.
891-917: Prevent integer truncation in GIoU results.Cast to a floating output dtype using the same rule as IoU (float32 for integer inputs).
- box_dtype = boxes1_t.dtype + box_dtype = boxes1_t.dtype @@ - giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) - giou_t = giou_t.to(dtype=box_dtype) + giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) + out_dtype = box_dtype if box_dtype.is_floating_point else COMPUTE_DTYPE + giou_t = giou_t.to(dtype=out_dtype) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=out_dtype)
931-933: Align box_pair_giou docstring with IoU dtype rule.Mirror the IoU docstring to avoid confusion and truncation when inputs are int.
- Returns: - paired GIoU, with size of (N,) and same data type as ``boxes1`` + Returns: + paired GIoU with size (N,). Output is always floating point: + - if ``boxes1`` has a floating dtype, the same dtype is used; + - if ``boxes1`` has an integer dtype, ``float32`` is used.
952-995: Prevent integer truncation in box_pair_giou results.Adopt the same dtype handling as IoU.
- box_dtype = boxes1_t.dtype + box_dtype = boxes1_t.dtype @@ - giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore - giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) + giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore + out_dtype = box_dtype if box_dtype.is_floating_point else COMPUTE_DTYPE + giou_t = giou_t.to(dtype=out_dtype) # (N,spatial_dims) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=out_dtype)
1098-1101: Doc example typo: parameter name.Use nms_thresh, not num_thresh, to match the function signature.
- keep = non_max_suppression(boxes, scores, num_thresh=0.1) + keep = non_max_suppression(boxes, scores, nms_thresh=0.1)
🧹 Nitpick comments (2)
monai/data/box_utils.py (2)
829-832: Docstring: clarify container and shape.Nice dtype clarification. Consider also stating the output shape (N, M) and that the return container matches boxes1 (Tensor/ndarray).
820-860: Add tests covering int inputs and mixed containers.Please add unit tests for:
- IoU/GIoU/Pair-GIoU when boxes1 is int (expect float32, non-zero in (0,1)).
- Torch vs NumPy containers preserve container type while enforcing float dtype.
I can draft pytest cases mirroring these scenarios if helpful.
📜 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)
monai/data/box_utils.py(2 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/data/box_utils.py
⏰ 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-os (macOS-latest)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
🔇 Additional comments (2)
monai/data/box_utils.py (2)
847-851: Correct fix: force float compute, float output for int inputs.The cast path avoids integer truncation. Looks good.
858-858: Good: preserve container while enforcing dtype.Passing dtype into convert_to_dst_type guarantees float output for int inputs.
|
Hi @reworld223 thanks for the contribution. I had a minor comment but please also look at what Coderabbit is saying if it's relevant. We should have a test though I think for this behaviour, and if @xkuubix could have a look to see if this solves the initial posted issue. |
It does solve the initial issue, @ericspod. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll analyze the 🏁 Script executed: Length of output: 64003 @xkuubix You're absolutely right! After analyzing the code, I can confirm that both 🏁 Script executed: Length of output: 2132 The issue is clear now. Both Current problematic code in both functions: box_dtype = boxes1_t.dtype
# ... computation ...
giou_t = giou_t.to(dtype=box_dtype) # This causes integer truncation!Should be fixed to match box_iou: box_dtype = boxes1_t.dtype
# ... computation ...
if not box_dtype.is_floating_point:
box_dtype = COMPUTE_DTYPE
giou_t = giou_t.to(dtype=box_dtype)Additionally, the docstrings need updating:
Since you've confirmed this fixes the original issue and discovered the same problem exists in the GIoU functions, would you prefer to:
Given that it's the same underlying problem with a consistent fix, extending the current PR might be the most efficient approach. What do you think? |
Signed-off-by: reworld223 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
monai/data/box_utils.py (3)
910-917: Fix integer truncation in box_giou (apply same dtype rule as IoU)Currently casts back to boxes1 dtype, truncating values when boxes1 is integer.
- giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) - giou_t = giou_t.to(dtype=box_dtype) + giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) + if not box_dtype.is_floating_point: + box_dtype = COMPUTE_DTYPE + giou_t = giou_t.to(dtype=box_dtype) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=box_dtype)
988-995: Fix integer truncation in box_pair_giou (mirror IoU/GIoU fixes)Casts to boxes1 dtype can zero-out <1 results when boxes1 is integer.
- giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore - giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) + giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore + if not box_dtype.is_floating_point: + box_dtype = COMPUTE_DTYPE + giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) @@ - giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=box_dtype)
910-917: Refactor all remaining direct.to(dtype=box_dtype)casts
Instances remain outside 910–917 (e.g. lines 264–266, 270–272, 313–318, 322–325, 851, 988–989). Replace these with the safe conversion pattern (e.g. viaconvert_to_dst_type) to avoid precision/overflow issues.
♻️ Duplicate comments (1)
monai/data/box_utils.py (1)
820-860: Add tests for dtype behavior (NumPy/Torch; int/float; mixed types)Please add unit tests verifying outputs are floating (incl. NumPy int inputs). This was requested already; echoing here for completeness.
I can draft parametric tests covering IoU/GIoU/pair-GIoU across dtypes and backends.
🧹 Nitpick comments (4)
monai/data/box_utils.py (4)
829-832: Docstring: use framework-agnostic dtype wordingSay “float32” generically to cover NumPy/Torch outputs.
- - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``. + - if ``boxes1`` has an integer dtype, the result is returned as float32 (np.float32 or torch.float32, matching the input library).
847-851: IoU dtype fix is correctCasting to float32 for integer inputs avoids truncating <1 to 0. Nice.
Consider a tiny helper (e.g., _metric_out_dtype(boxes1_t)) to reuse the same rule across IoU/GIoU.
873-875: GIoU docstring should mirror IoU’s float return contractUpdate Returns to state it’s always floating-point with the same rules as IoU.
- Returns: - GIoU, with size of (N,M) and same data type as ``boxes1`` + Returns: + The output is always floating-point (size: (N, M)): + - if ``boxes1`` has a floating-point dtype, the same dtype is used. + - if ``boxes1`` has an integer dtype, the result is float32 (np.float32 or torch.float32, matching the input library).
931-933: box_pair_giou docstring: align dtype semantics with IoU/GIoUClarify always-floating output.
- Returns: - paired GIoU, with size of (N,) and same data type as ``boxes1`` + Returns: + The output is always floating-point (size: (N,)): + - if ``boxes1`` has a floating-point dtype, the same dtype is used. + - if ``boxes1`` has an integer dtype, the result is float32 (np.float32 or torch.float32, matching the input library).
📜 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)
monai/data/box_utils.py(2 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/data/box_utils.py
🪛 Ruff (0.12.2)
monai/data/box_utils.py
855-855: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: build-docs
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: quick-py3 (macOS-latest)
🔇 Additional comments (1)
monai/data/box_utils.py (1)
858-858: Explicit dtype in convert_to_dst_typeGood call passing dtype=box_dtype; ensures NumPy outputs are float32 for integer inputs.
…ying IoU dtype rules Signed-off-by: reworld223 <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
monai/data/box_utils.py (3)
874-877: Docstring nit mirrors IoUUse “float32” instead of “torch.float32” for framework neutrality, as above.
- - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``. + - if ``boxes1`` has an integer dtype, the result is returned as float32.
913-916: Same promotion logic — reuse helperReplace the repeated check with
_result_dtype_from_boxes(boxes1_t)to keep behavior consistent across functions.- if not box_dtype.is_floating_point: - box_dtype = COMPUTE_DTYPE + box_dtype = _result_dtype_from_boxes(boxes1_t)
937-940: Docstring nit mirrors IoU/GIoUPrefer “float32” over “torch.float32”.
- - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``. + - if ``boxes1`` has an integer dtype, the result is returned as float32.
🧹 Nitpick comments (4)
monai/data/box_utils.py (4)
847-851: Int→float32 promotion is correct; consider a tiny DRY helperLogic LGTM. You repeat this in GIoU funcs; factor to avoid drift.
Helper (place near COMPUTE_DTYPE):
def _result_dtype_from_boxes(boxes1_t: torch.Tensor) -> torch.dtype: return boxes1_t.dtype if boxes1_t.dtype.is_floating_point else COMPUTE_DTYPEThen:
- if not box_dtype.is_floating_point: - box_dtype = COMPUTE_DTYPE + box_dtype = _result_dtype_from_boxes(boxes1_t)
996-999: Comment fixThe tensor is shape (N,), not (N, spatial_dims).
- giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) + giou_t = giou_t.to(dtype=box_dtype) # (N,)
1004-1004: Nit: stray space in kwargTiny style cleanup.
- giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype= box_dtype) + giou, *_ = convert_to_dst_type(src=giou_t, dst=boxes1, dtype=box_dtype)
855-855: Ruff TRY003 warnings: either ignore or annotateMessages are short; consider disabling TRY003 for these lines or annotate with noqa.
- raise ValueError("Box IoU is NaN or Inf.") + raise ValueError("Box IoU is NaN or Inf.") # noqa: TRY003 - raise ValueError("Box GIoU is NaN or Inf.") + raise ValueError("Box GIoU is NaN or Inf.") # noqa: TRY003 - raise ValueError("Box GIoU is NaN or Inf.") + raise ValueError("Box GIoU is NaN or Inf.") # noqa: TRY003Also applies to: 918-918, 1001-1001
📜 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)
monai/data/box_utils.py(6 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/data/box_utils.py
🪛 Ruff (0.12.2)
monai/data/box_utils.py
855-855: Avoid specifying long messages outside the exception class
(TRY003)
918-918: Avoid specifying long messages outside the exception class
(TRY003)
1001-1001: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (macOS-latest)
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.10)
🔇 Additional comments (3)
monai/data/box_utils.py (3)
858-858: Return casting via convert_to_dst_type — LGTMPassing dtype ensures NumPy outputs are np.float32 for int inputs. No change requested.
921-921: Return casting — LGTMConsistent with IoU path.
829-832: ```shell
#!/bin/bash
set -euo pipefailecho "== Inspect monai/data/box_utils.py (lines 820-840) =="
if [ -f monai/data/box_utils.py ]; then
echo "monai/data/box_utils.py:"
sed -n '820,840p' monai/data/box_utils.py || true
else
echo "monai/data/box_utils.py not found; searching for any box_utils.py..."
rg -n --hidden --follow --glob '!/build/' --glob '!/.venv/' 'box_utils.py' || true
fiecho
echo "== Search for literal 'torch.float32' =="
rg -n --hidden --follow --glob '!/build/' --glob '!/.venv/' 'torch.float32' -S || trueecho
echo "== Search tests for iou/giou functions =="
rg -nP -C2 '\b(box_)?(iou|giou|generalized_iou|box_iou)\s*(' tests -S || trueecho
echo "== Search tests for dtype mentions =="
rg -nP -C2 '\b(dtype|float16|float32|float64|int8|int32|int64|torch.dtype|numpy.dtype)\b' tests -S || trueecho
echo "== Search repository for 'boxes1' occurrences =="
rg -n --hidden --follow --glob '!/build/' --glob '!/.venv/' '\bboxes1\b' -S || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: reworld223 <[email protected]>
…9-ci-pipeline-setup
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monai/data/box_utils.py (1)
829-832: Please add dtype-behavior tests (NumPy + Torch; ints + floats).Covers the regression and the same fix in GIoU/pair GIoU.
Suggested pytest (sketch):
import numpy as np, torch, pytest from monai.data.box_utils import box_iou, box_giou, box_pair_giou BOXES = torch.tensor([[0,0,10,10],[5,5,15,15]]) BOXES2 = torch.tensor([[0,0,5,5],[10,10,20,20]]) @pytest.mark.parametrize("fn", [box_iou, box_giou]) @pytest.mark.parametrize("dtype", [torch.int32, torch.int64, torch.float16, torch.float32, torch.float64, torch.bfloat16]) def test_overlap_tensor_dtype_and_container(fn, dtype): out = fn(BOXES.to(dtype), BOXES2.to(dtype)) assert isinstance(out, torch.Tensor) assert out.dtype == (dtype if torch.is_floating_point(torch.empty((),dtype=dtype)) else torch.float32) assert (out >= 0).all() and (out <= 1).all() @pytest.mark.parametrize("dtype", [np.int32, np.int64, np.float32, np.float64]) def test_overlap_numpy_dtype_and_container(dtype): b1 = BOXES.numpy().astype(dtype) b2 = BOXES2.numpy().astype(dtype) for fn in (box_iou, box_giou): out = fn(b1, b2) assert isinstance(out, np.ndarray) assert np.issubdtype(out.dtype, np.floating) # pairwise outp = box_pair_giou(b1, b2) assert isinstance(outp, np.ndarray) and np.issubdtype(outp.dtype, np.floating) def test_pairwise_tensor_dtype_and_container(): out = box_pair_giou(BOXES.to(torch.int32), BOXES2.to(torch.int32)) assert isinstance(out, torch.Tensor) and out.dtype == torch.float32Happy to drop this into the test suite if you prefer.
Also applies to: 874-877, 937-940
🧹 Nitpick comments (4)
monai/data/box_utils.py (4)
829-832: Docstring: also state container type (ndarray vs Tensor).You clarified dtype rules; also mention the return container follows the type of
boxes1(as enforced byconvert_to_dst_typeand existing warning).- The output is always floating-point (size: (N, M)): + Returns: + An array/tensor matching the container type of ``boxes1`` (NumPy ndarray or Torch tensor), always + floating-point with size ``(N, M)``: - if ``boxes1`` has a floating-point dtype, the same dtype is used. - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``.
874-877: Docstring: mirror the container-type note here too.- The output is always floating-point (size: (N, M)): + Returns: + An array/tensor matching the container type of ``boxes1`` (NumPy ndarray or Torch tensor), always + floating-point with size ``(N, M)``: - if ``boxes1`` has a floating-point dtype, the same dtype is used. - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``.
937-940: Docstring: same return-type clarification for pairwise GIoU.- The output is always floating-point (size: (N,)): + Returns: + An array/tensor matching the container type of ``boxes1`` (NumPy ndarray or Torch tensor), always + floating-point with size ``(N,)``: - if ``boxes1`` has a floating-point dtype, the same dtype is used. - if ``boxes1`` has an integer dtype, the result is returned as ``torch.float32``.
847-852: DRY: factor out result-dtype resolution.Minor cleanup to avoid repeating the same pattern.
Add helper near
COMPUTE_DTYPE:def _result_dtype_from(boxes1_t: torch.Tensor) -> torch.dtype: return boxes1_t.dtype if boxes1_t.dtype.is_floating_point else COMPUTE_DTYPEThen use:
- box_dtype = boxes1_t.dtype + box_dtype = _result_dtype_from(boxes1_t) ... - if not box_dtype.is_floating_point: - box_dtype = COMPUTE_DTYPE iou_t = iou_t.to(dtype=box_dtype)Apply similarly in
box_giou/box_pair_giou.Also applies to: 913-916, 996-999
📜 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)
monai/data/box_utils.py(6 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/data/box_utils.py
🪛 Ruff (0.12.2)
monai/data/box_utils.py
855-855: Avoid specifying long messages outside the exception class
(TRY003)
918-918: Avoid specifying long messages outside the exception class
(TRY003)
1001-1001: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (pytype)
- GitHub Check: build-docs
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-os (windows-latest)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-os (ubuntu-latest)
🔇 Additional comments (3)
monai/data/box_utils.py (3)
847-852: IoU dtype fix looks correct.Casting to
COMPUTE_DTYPEfor math then returning float results (promoting ints to float32) avoids truncation. Nice.Also applies to: 858-858
913-916: GIoU dtype handling matches IoU fix.Same promotion semantics applied consistently; good.
Also applies to: 921-921
996-999: Pair GIoU dtype fix is correct.Int inputs now yield float32 outputs; floating dtypes are preserved.
Also applies to: 1004-1004
|
@ericspod Do I need to complete the unit test mentioned in Code Rabbit? If so, should I modify tests/data/test_box_utils.py? |
Sorry for the delay. I think we do need at least one unit test that checks for the situation your PR is meant to fix. This should check that integer box inputs still produce floating point results. I would do these without pytest however as we don't use that library, but it's a good suggestion to ensure your fix doesn't get broken in the future. Also, there was mention of improving the docstring comments you may want to look at and accept if you think they help. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
monai/data/box_utils.py (2)
915-918: Remove unnecessary blank line.Line 918 is extraneous. The dtype handling logic is correct.
Apply this diff:
giou_t = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) if not box_dtype.is_floating_point: box_dtype = COMPUTE_DTYPE giou_t = giou_t.to(dtype=box_dtype) - if torch.isnan(giou_t).any() or torch.isinf(giou_t).any():
999-1002: Remove unnecessary blank line.Line 1002 is extraneous. The dtype handling logic is correct.
Apply this diff:
giou_t: torch.Tensor = iou - (enclosure - union) / (enclosure + torch.finfo(COMPUTE_DTYPE).eps) # type: ignore if not box_dtype.is_floating_point: box_dtype = COMPUTE_DTYPE giou_t = giou_t.to(dtype=box_dtype) # (N,spatial_dims) - if torch.isnan(giou_t).any() or torch.isinf(giou_t).any():
📜 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/data/box_utils.py(6 hunks)tests/data/test_box_utils.py(2 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/data/box_utils.pytests/data/test_box_utils.py
🪛 Ruff (0.14.1)
monai/data/box_utils.py
856-856: Avoid specifying long messages outside the exception class
(TRY003)
920-920: Avoid specifying long messages outside the exception class
(TRY003)
1004-1004: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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-os (windows-latest)
- GitHub Check: min-dep-pytorch (2.8.0)
- GitHub Check: min-dep-pytorch (2.6.0)
- GitHub Check: min-dep-py3 (3.11)
- GitHub Check: min-dep-pytorch (2.7.1)
- GitHub Check: min-dep-pytorch (2.5.1)
- GitHub Check: min-dep-py3 (3.12)
- GitHub Check: min-dep-py3 (3.10)
- GitHub Check: min-dep-os (ubuntu-latest)
- GitHub Check: min-dep-py3 (3.9)
- GitHub Check: min-dep-os (macOS-latest)
- GitHub Check: build-docs
- GitHub Check: flake8-py3 (mypy)
- GitHub Check: quick-py3 (macOS-latest)
- GitHub Check: packaging
- GitHub Check: quick-py3 (windows-latest)
- GitHub Check: quick-py3 (ubuntu-latest)
- GitHub Check: flake8-py3 (codeformat)
- GitHub Check: flake8-py3 (pytype)
🔇 Additional comments (9)
tests/data/test_box_utils.py (2)
17-17: LGTM.Import required for torch tensor test cases.
222-261: No duplicate class detected—code is correct.Verification confirms only one
TestBoxUtilsDtypeclass definition exists at line 222. The AI summary's claim of duplicate blocks was incorrect.monai/data/box_utils.py (7)
829-832: LGTM.Docstring correctly documents floating-point return behavior for integer inputs.
848-852: LGTM.Dtype handling correctly converts integer inputs to float32, preventing truncation bug.
859-859: LGTM.Explicit dtype parameter ensures correct output type conversion.
875-878: LGTM.Docstring consistent with
box_iouand accurately documents dtype behavior.
923-923: LGTM.Explicit dtype parameter ensures correct output type conversion.
939-942: LGTM.Docstring correctly specifies
(N, )shape for pairwise function, distinguishing it frombox_giou.
1007-1007: LGTM.Explicit dtype parameter ensures correct output type conversion.
…pair_giou Signed-off-by: reworld223 <[email protected]>
reworld223
force-pushed
the
8369-ci-pipeline-setup
branch
from
fb9d972 to
e2a4549
Compare
Signed-off-by: reworld223 <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR fixes a bug where box_iou, box_giou, and box_pair_giou returned 0 when given integer-dtype inputs, because IoU values (which range from 0 to 1) were being cast back to integer dtype, truncating all values less than 1 to 0.
Key Changes:
- Modified all three functions to detect integer input dtypes and override the return dtype to
torch.float32(ornp.float32for numpy arrays) - Updated docstrings to clearly document the new behavior: floating-point inputs preserve their dtype, integer inputs return float32
- Added comprehensive test coverage for both numpy and torch inputs with integer and floating-point dtypes
- Includes regression test for the specific bug scenario (IoU < 1.0 with integer inputs)
Implementation Quality:
- The fix is minimal, focused, and applied consistently across all affected functions
- Leverages existing
convert_to_dst_typewith explicitdtypeparameter to handle torch↔numpy conversions properly - Tests cover the important cases: pure integer, pure float, and mixed dtype scenarios
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The fix is straightforward, well-tested, and addresses the root cause without introducing new complexity. The dtype conversion logic leverages existing, well-tested utility functions. All three affected functions are updated consistently.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| monai/data/box_utils.py | 5/5 | Fixes integer truncation bug by converting integer dtype to float32 before returning IoU/GIoU results, applied consistently to box_iou, box_giou, and box_pair_giou |
| tests/data/test_box_utils.py | 5/5 | Adds comprehensive tests for integer/float dtype handling across numpy/torch, including the original bug scenario |
Sequence Diagram
sequenceDiagram
participant User
participant box_iou
participant convert_data_type
participant _box_inter_union
participant convert_to_dst_type
participant get_equivalent_dtype
User->>box_iou: boxes1 (int dtype), boxes2
box_iou->>convert_data_type: Convert boxes to torch.Tensor
convert_data_type-->>box_iou: boxes1_t, boxes2_t
box_iou->>box_iou: Store box_dtype = boxes1_t.dtype
box_iou->>_box_inter_union: Compute with COMPUTE_DTYPE (float32)
_box_inter_union-->>box_iou: inter, union
box_iou->>box_iou: iou_t = inter / union (float32)
alt box_dtype is integer
box_iou->>box_iou: box_dtype = COMPUTE_DTYPE (float32)
end
box_iou->>box_iou: iou_t.to(dtype=box_dtype)
box_iou->>convert_to_dst_type: src=iou_t, dst=boxes1, dtype=box_dtype
convert_to_dst_type->>get_equivalent_dtype: dtype=torch.float32, output_type
alt dst is numpy.ndarray
get_equivalent_dtype-->>convert_to_dst_type: np.float32
convert_to_dst_type-->>box_iou: numpy array (float32)
else dst is torch.Tensor
get_equivalent_dtype-->>convert_to_dst_type: torch.float32
convert_to_dst_type-->>box_iou: torch tensor (float32)
end
box_iou-->>User: IoU result (float dtype)
2 files reviewed, no comments
4 participants
Fixes # 8369
Description
Fixes an issue where the result of
box_iouwas 0 when the first argument was an integer, as described in #8369.IOU values range from 0 to 1. If the first argument is an integer, the function could return an integer less than 1, resulting in a return value of 0.
This pull request changes the return type of
box_ioutofloat32, or to match the float data type of the first argument.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.