Fix box_iou returning 0 for floating-point results less than 1. #8369 #8553
Conversation
reworld223
requested review from
KumoLiu,
ericspod and
Nic-Ma
as code owners
WalkthroughChanges in monai/data/box_utils.py affecting three public functions: box_iou, box_giou, and box_pair_giou. Docstrings updated to state outputs are always floating and to specify dtype rules: if Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 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.
|
|
||
| # convert tensor back to numpy if needed | ||
| iou, *_ = convert_to_dst_type(src=iou_t, dst=boxes1) | ||
| iou, *_ = convert_to_dst_type(src=iou_t, dst=boxes1, dtype=box_dtype) |
There was a problem hiding this comment.
Will this return a Numpy array if the inputs are arrays? We should have a test for this new behaviour to verify the result is a float array/tensor even when the inputs are integer boxes.
There was a problem hiding this comment.
Yes, I checked, box_iou works as expected. It correctly handles NumPy arrays and Torch tensors with int or float dtypes, including mixed types, and always returns a float array/tensor.
If the inputs are mixed types (one NumPy array, one Torch tensor), box_iou raises a UserWarning indicating that the result type follows the type of the first input, e.g.:
boxes1 is <class 'numpy.ndarray'>, while boxes2 is <class 'torch.Tensor'>. The result will be <class 'numpy.ndarray'>. or vice versa.
|
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? |
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.