Skip to content

perf(RandCropBoxByPosNegLabeld): avoid N-fold mask allocation in randomize#8993

Open
aymuos15 wants to merge 3 commits into
Project-MONAI:devfrom
aymuos15:perf/randcropboxbyposneglabeld-randomize
Open

perf(RandCropBoxByPosNegLabeld): avoid N-fold mask allocation in randomize#8993
aymuos15 wants to merge 3 commits into
Project-MONAI:devfrom
aymuos15:perf/randcropboxbyposneglabeld-randomize

Conversation

@aymuos15

@aymuos15 aymuos15 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Description

RandCropBoxByPosNegLabeld.randomize builds a foreground/background mask when precomputed indices are not available. The previous implementation called convert_box_to_mask for each box (creating an N-channel stacked mask) then reduced with np.amax to a single channel, allocating an (N_boxes, H, W, D) int16 intermediate that grows linearly with the number of boxes.

The fix paints directly into a single (1, H, W, D) union mask by iterating over each extended box and setting mask[0, box_slices] = 1.

Changes

monai/apps/detection/transforms/dictionary.py

  1. Remove the now-unused import of convert_box_to_mask from monai.apps.detection.transforms.box_ops.

  2. Replace the convert_box_to_mask + np.amax(...)[0:1] block in randomize() with direct slicing into a pre-allocated zero mask.

Performance

Benchmarked old (convert_box_to_mask + np.amax) vs new (direct slicing) implementations on synthetic 3D volumes with random boxes (spatial_size=(64,64,64), num_samples=4, pos=1, neg=1, whole_box=True).

System specs

  • CPU: 12th Gen Intel Core i7-12800H (14 cores / 20 threads, up to 4.8 GHz)
  • RAM: 30 GiB
  • OS: Ubuntu 22.04.5 LTS, kernel 6.8.0-124-generic
  • Python 3.10.12, NumPy 2.2.6, PyTorch 2.13.0+cu130

randomize() only

image_size n_boxes old (ms) new (ms) speedup
(96,96,96) 5 2.471 1.331 1.86x
(96,96,96) 20 17.998 1.921 9.37x
(160,160,160) 5 22.931 15.241 1.50x
(160,160,160) 20 78.651 6.563 11.98x
(256,256,128) 5 62.477 26.386 2.37x
(256,256,128) 20 174.831 24.427 7.16x

Full __call__()

image_size n_boxes old (ms) new (ms) speedup
(96,96,96) 5 6.416 4.823 1.33x
(96,96,96) 20 21.745 5.078 4.28x
(160,160,160) 5 22.820 13.778 1.66x
(160,160,160) 20 85.259 14.000 6.09x
(256,256,128) 5 76.623 38.455 1.99x
(256,256,128) 20 188.849 36.955 5.11x

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

…omize

Replace convert_box_to_mask + amax with direct per-box slicing into a
single (1,H,W,D) union mask. This eliminates the (N_boxes,H,W,D) int16
intermediate, reducing peak memory and improving randomize() throughput
by ~2-3x on typical 3D volumes.

- randomize():  2.05-2.95x speedup (3 configs, interleaved medians)
- full __call__: 1.81-2.49x speedup
- Numerics: bit-identical (mask, indices, centers all equal)
- Peak RSS: unchanged (fg/bg index arrays dominate)

Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

RandCropBoxByPosNegLabeld.randomize now constructs a zero-valued mask, marks each extended-box region directly, and passes it through map_binary_to_indices to derive foreground and background indices. The unused convert_box_to_mask import was removed.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change: optimizing RandCropBoxByPosNegLabeld.randomize by avoiding repeated mask allocation.
Description check ✅ Passed The description covers the change, motivation, benchmarks, and change type checklist, matching the template well.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
monai/apps/detection/transforms/dictionary.py (1)

1157-1163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider iterable unpacking per RUF005.

Ruff flags two concatenation patterns that could use unpacking for clarity.

♻️ Optional refactor
-            mask_img = np.zeros((1,) + tuple(image_size), dtype=np.int16)
+            mask_img = np.zeros((1, *tuple(image_size)), dtype=np.int16)
-                slicing = (0,) + tuple(
-                    slice(extended_boxes_np[b, d], extended_boxes_np[b, d + spatial_dims]) for d in range(spatial_dims)
-                )
+                slicing = (0, *tuple(
+                    slice(extended_boxes_np[b, d], extended_boxes_np[b, d + spatial_dims]) for d in range(spatial_dims)
+                ))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@monai/apps/detection/transforms/dictionary.py` around lines 1157 - 1163,
Update the tuple construction in the mask-building loop to use iterable
unpacking instead of concatenating the leading `(0,)` tuple with the generated
slice tuple, resolving the RUF005 warning while preserving the existing
`mask_img` indexing behavior.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@monai/apps/detection/transforms/dictionary.py`:
- Around line 1157-1163: Update the tuple construction in the mask-building loop
to use iterable unpacking instead of concatenating the leading `(0,)` tuple with
the generated slice tuple, resolving the RUF005 warning while preserving the
existing `mask_img` indexing behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ad894203-292e-4393-874c-6c47e55e3882

📥 Commits

Reviewing files that changed from the base of the PR and between fd12fdc and 916ede3.

📒 Files selected for processing (1)
  • monai/apps/detection/transforms/dictionary.py

…ransforms

Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant