-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement convex_hull_image
and convex_hull_object
#828
base: branch-25.04
Are you sure you want to change the base?
Conversation
* observed that a 2D planar object within a 3D volume can cause QHull to fail * should also be more efficient to process such 'thin' objects in a reduced dimension * original shape is restored for the returned convex hull image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @grlee77 for the convex hall implementations!
It looks good to me in general. I left some feedback and suggestions in the comments.
"Returning empty image", | ||
UserWarning, | ||
) | ||
return np.zeros(image.shape, dtype=bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to return CuPy array here?
return np.zeros(image.shape, dtype=bool) | |
return cp.zeros(image.shape, dtype=bool) |
raise ValueError("`connectivity` must be between 1 and image.ndim.") | ||
|
||
labeled_im = label(image, connectivity=connectivity, background=0) | ||
convex_obj = cp.zeros(image.shape, dtype=bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable is always overwritten or not used so can be removed.
convex_obj = cp.zeros(image.shape, dtype=bool) |
chimage3d = convex_hull_image( | ||
image3d, offset_coordinates=True, cpu_fallback_threshold=0 | ||
) | ||
chimage3d.sum() > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chimage3d.sum() > 0 | |
assert chimage3d.sum() > 0 |
The parameters listed under "Extra Parameters" above are present only | ||
in cuCIM and not in scikit-image. | ||
""" | ||
if connectivity not in tuple(range(1, image.ndim + 1)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no validation that the input is 2D or 3D. While the error will be caught by the connectivity check, it might be clearer to add an explicit dimensionality check:
if connectivity not in tuple(range(1, image.ndim + 1)): | |
if image.ndim not in (2, 3): | |
raise ValueError("Input image must be 2D or 3D") | |
if connectivity not in tuple(range(1, image.ndim + 1)): |
.. [1] https://blogs.mathworks.com/steve/2011/10/04/binary-image-convex-hull-algorithm-notes/ | ||
|
||
""" | ||
if cpu_fallback_threshold is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding validation that the threshold is non-negative when provided:
if cpu_fallback_threshold is None: | |
if cpu_fallback_threshold is not None and cpu_fallback_threshold < 0: | |
raise ValueError("cpu_fallback_threshold must be non-negative") | |
if cpu_fallback_threshold is None: |
to numerical floating point errors, a tolerance of 0 can result in | ||
some points erroneously being classified as being outside the hull. | ||
include_borders: bool, optional | ||
If ``False``, vertices/edges are excluded from the final hull mask. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since include_borders=False
case raises the NotImplementedError, it might be good to document this in the docstring.
def _offsets_diamond(ndim): | ||
offsets = np.zeros((2 * ndim, ndim)) | ||
for vertex, (axis, offset) in enumerate(product(range(ndim), (-0.5, 0.5))): | ||
offsets[vertex, axis] = offset | ||
return cp.asarray(offsets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if np.zeros()
is used here because calculating offsets
in system memory and then converting them to GPU memory is more efficient than creating or manipulating a CuPy array.
# float32 will be used if coord_dtype is <= 16-bit | ||
# otherwise, use float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is slightly misleading as promote_types behavior depends on the specific dtype, not just its size. It would be better to explicitly document the exact promotion rules or use more explicit logic.
.. [1] https://blogs.mathworks.com/steve/2011/10/04/binary-image-convex-hull-algorithm-notes/ | ||
|
||
""" | ||
if cpu_fallback_threshold is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if cpu_fallback_threshold is None: | |
if tolerance <= 0: | |
raise ValueError("tolerance must be positive") | |
if cpu_fallback_threshold is None: |
This MR implements a hybrid CPU + GPU implementation of
convex_hull_image
andconvex_hull_object
. These functions can be used standalone, butconvex_hull_image
is also used during computation of some regionprops:area_convex
image_convex
feret_diameter_max
solidity
The implementation here still uses QHull via SciPy on the CPU, but all preprocessing computations as well as the post-processing of which image pixels fall within the convex hull polygon are done on the GPU. Fortunately, QHull was not the bottleneck, as seen in the benchmark results below.
The image below shows an example of a non-convex labeled region (yellow) with the convex region computed by

convex_hull_image
shown in redBenchmark results
The last two columns labeled "accel" are the ratio of (
cpu_duration / gpu_duration
). "32" indicates the GPU case was run with an extra kwarg that allows 32-bit floating point when computing the final convex image mask from the convex hull coordinates. All durations are in seconds.All except the smallest sizes tested show performance benefit relative to scikit-image. The relative performance benefit can be very large, especially for large images and in cases where the image is highly concave and if
offset_coords=False
as this reduces the run time of the QHull algorithm on the CPU.benchmarking script