adding sub_mask function to plantcv.plantcv#1864
Conversation
joshqsumner
added
new feature
 |
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 30, 2026 10:16p.m. | Review ↗ |
| Test coverage | |
Mar 30, 2026 10:16p.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (New Code) | Line Coverage (Overall) |
|---|---|---|
| Aggregate | 100% [✓ above threshold] |
99.9% [▼ down 0.1% from main] |
| Python | 100% [✓ above threshold] |
99.9% [▼ down 0.1% from main] |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
joshqsumner
added
ready to review
and removed
work in progress
There was a problem hiding this comment.
Pull request overview
Adds a new sub_mask feature to PlantCV for sampling multiple non-overlapping circular ROIs within an existing object mask (Spot Sampling, closes #1863).
Changes:
- Introduces
plantcv.plantcv.sub_maskimplementation for generating labeled circular sub-masks. - Exposes
sub_maskviaplantcv/plantcv/__init__.pyand adds user documentation + MkDocs nav entry. - Adds initial unit tests for expected sub-mask behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
plantcv/plantcv/submask.py |
New implementation of random circular sub-mask sampling and helpers. |
plantcv/plantcv/__init__.py |
Exports sub_mask in the public plantcv.plantcv API. |
tests/plantcv/test_sub_mask.py |
Adds tests for successful placement and “cannot place” scenarios. |
docs/sub_mask.md |
New documentation page for sub_mask. |
mkdocs.yml |
Adds the new docs page to site navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Find coordinates in mask where mask is non-zero | ||
| coords = np.column_stack(np.where(mask > 0)) | ||
| # Randomly select a center point from these coordinates | ||
| center = coords[random.randint(0, len(coords) - 1)] | ||
| x, y = center[1], center[0] | ||
| # Create an ROI from the random center point | ||
| spot = circle(img=img, x=x, y=y, r=radius) | ||
| return spot |
There was a problem hiding this comment.
_make_random_circle can raise at runtime in two common cases: (1) if mask has no non-zero pixels then len(coords) == 0 and random.randint(0, -1) will error; (2) if a non-zero pixel is near the image border, roi_methods.circle will raise (fatal_error) because the ROI extends outside the image. Consider filtering candidate centers to those that are at least radius pixels from the image edges and returning early (or warning) when no valid centers exist.
| # Create an empty mask | ||
| labeled_mask = np.zeros_like(mask) |
There was a problem hiding this comment.
labeled_mask = np.zeros_like(mask) inherits the input mask dtype (commonly uint8), so labels can overflow/wrap once sample_num > 255. PlantCV labeled masks elsewhere use np.int32 (e.g., plantcv/plantcv/masks2labels.py:32). Consider initializing labeled_mask as a 2D np.int32 array matching mask.shape[:2].
| # Create an empty mask | |
| labeled_mask = np.zeros_like(mask) | |
| # Create an empty labeled mask with sufficient integer range for labels | |
| labeled_mask = np.zeros(mask.shape[:2], dtype=np.int32) |
| tries = 0 | ||
| sample_num = 0 | ||
| while len(np.unique(labeled_mask)) - 1 < num_masks: | ||
| tries += 1 | ||
| spot = _make_random_circle(img=img, mask=mask, radius=radius) | ||
| spot_mask = roi2mask(img=img, roi=spot) | ||
| within = _is_mask_within(full_mask=mask, submask=spot_mask) | ||
| if within and _check_overlapping_masks(labeled_mask, spot_mask): |
There was a problem hiding this comment.
The loop condition recalculates np.unique(labeled_mask) on every iteration, which scans the full array repeatedly and can be expensive on large masks. Since you already track sample_num, you can drive the loop with while sample_num < num_masks: (or maintain a counter) to avoid repeated full-array uniques.
| """ | ||
| Make circular sub-masks inside the mask of an object | ||
|
|
||
| Parameters | ||
| ---------- | ||
| img = numpy.ndarray, input image | ||
| mask = numpy.ndarray, binary mask of object | ||
| num_masks = int, number of circular sub-masks to make | ||
| radius = int, radius of circular mask to make. Defaults to 5. | ||
|
|
||
| Returns | ||
| ------- | ||
| labeled_mask = numpy.ndarray, labelled mask of circular masks each within complete mask. | ||
| """ |
There was a problem hiding this comment.
The sub_mask docstring uses a one-off parameter format (img = numpy.ndarray, ...) that doesn’t match the NumPy-style name : type format used widely in this repo (e.g., plantcv/plantcv/masks2labels.py:10-27). Aligning the docstring style will make API docs more consistent and easier to parse.
| def test_sub_mask_success(test_data): | ||
| """Test for PlantCV.""" | ||
| img = cv2.imread(test_data.small_rgb_img, -1) | ||
| mask = cv2.imread(test_data.small_bin_img, -1) | ||
| spots = sub_mask(img, mask, 2, 2) | ||
| assert len(np.unique(spots)) == 3 |
There was a problem hiding this comment.
These tests assert exact label counts, but sub_mask is inherently random and currently has no way to provide a deterministic seed/RNG. This can make CI flaky if a different set of random centers results in a different number of placed spots. Consider adding an optional seed/rng parameter to sub_mask (similar to io.random_subset) and setting it in the tests.
| **plantcv.plantcv.sub_mask**(*img, mask, num_masks=1, radius=5*) | ||
|
|
||
| **returns** A `numpy.ndarray` labelled mask. | ||
|
|
||
| - **Parameters:** | ||
| - img - Grayscale or RGB image. | ||
| - mask - Binary mask of the image. | ||
| - num_masks - A number of circular regions to make masks of, defaults to 1. These spots cannot overlap, so specifying too many may trigger a warning that fewer masks could be placed than were specified. | ||
| - radius - Radius of the circular region(s) to select. These spots cannot overlap, so specifying too large of a radius may trigger a warning that fewer masks could be placed than were specified. | ||
|
|
||
|
|
||
| - **Context:** | ||
| - Used to downsample an image, particularly for spectral analysis. | ||
|
|
||
|
|
||
| - **Example use:** | ||
| - Below |
There was a problem hiding this comment.
This page doesn’t follow the established docs pattern used elsewhere (e.g., docs/auto_crop.md): it uses plantcv.plantcv.sub_mask instead of plantcv.sub_mask, and several list items are indented with tabs which can render as code blocks in Markdown. Consider switching to the standard **plantcv.sub_mask**(...) heading and replacing tabs with spaces for consistent rendering.
| **plantcv.plantcv.sub_mask**(*img, mask, num_masks=1, radius=5*) | |
| **returns** A `numpy.ndarray` labelled mask. | |
| - **Parameters:** | |
| - img - Grayscale or RGB image. | |
| - mask - Binary mask of the image. | |
| - num_masks - A number of circular regions to make masks of, defaults to 1. These spots cannot overlap, so specifying too many may trigger a warning that fewer masks could be placed than were specified. | |
| - radius - Radius of the circular region(s) to select. These spots cannot overlap, so specifying too large of a radius may trigger a warning that fewer masks could be placed than were specified. | |
| - **Context:** | |
| - Used to downsample an image, particularly for spectral analysis. | |
| - **Example use:** | |
| - Below | |
| **plantcv.sub_mask**(*img, mask, num_masks=1, radius=5*) | |
| **returns** A `numpy.ndarray` labelled mask. | |
| - **Parameters:** | |
| - img - Grayscale or RGB image. | |
| - mask - Binary mask of the image. | |
| - num_masks - A number of circular regions to make masks of, defaults to 1. These spots cannot overlap, so specifying too many may trigger a warning that fewer masks could be placed than were specified. | |
| - radius - Radius of the circular region(s) to select. These spots cannot overlap, so specifying too large of a radius may trigger a warning that fewer masks could be placed than were specified. | |
| - **Context:** | |
| - Used to downsample an image, particularly for spectral analysis. | |
| - **Example use:** | |
| - Below |
Describe your changes
Adds
sub_maskfunction toplantcv.plantcvto sample N circular ROIs from a mask.Type of update
This is a new feature.
Associated issues
Closes #1863
For the reviewer
See this page for instructions on how to review the pull request.
plantcv/mkdocs.ymlupdating.md