Fast 2d convex hull #5155
Fast 2d convex hull #5155
Conversation
… removed imports and code for deprecation
|
Hello @lmmx! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
| @@ -88,6 +107,11 @@ def convex_hull_image(image, offset_coordinates=True, tolerance=1e-10): | |||
| Tolerance when determining whether a point is inside the hull. Due | |||
| to numerical floating point errors, a tolerance of 0 can result in | |||
| some points erroneously being classified as being outside the hull. | |||
| fast_drawing : bool or None, optional | |||
| If ``True``, use the fast SciPy function ``polygon_perimeter`` to locate | |||
There was a problem hiding this comment.
not sure I understand here, polygon_perimeter is a scikit-image function, not a scipy one? I had the impression that the fast scipy function was binary_fill_holes.
| def _apply_partial_offsets(img, coords, offsets): | ||
| """ | ||
| Apply the offsets only to the non-edge pixels, along with the trivial zero-offset. | ||
| """ |
There was a problem hiding this comment.
This function is a bit hard to read, could you please add a minimal example here in the docstring?
|
Hi @lmmx thanks a lot for the PR! A couple of points:
However, we first need to decide what / if we should deprecate. In #2928 there were a couple of tests on simple and more complex images which showed that there were some differences between the old and the new mode, both for import numpy as np
bw = np.array([
[0, 0, 0, 0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, 1, 0, 0, 0, 0],
[0, 0, 0, 1, 0, 1, 0, 0, 0],
[0, 0, 1, 0, 0, 0, 1, 0, 0],
[0, 1, 0, 0, 0, 0, 0, 1, 0],
[0, 0, 0, 0, 0, 0, 0, 0, 0],
[0, 0, 0, 0, 0, 0, 0, 0, 0],
])
out_1 = morphology.convex_hull_image(bw).astype(np.uint8)
out_2 = morphology.convex_hull_image(bw, fast_drawing=True, offset_coordinates=False).astype(np.uint8)I got the impression that your bug fix had fixed the difference between the two modes for |
| edge_t, edge_b = [coords[:,0] == lim for lim in (0, row_max)] | ||
| edge_l, edge_r = [coords[:,1] == lim for lim in (0, col_max)] | ||
| edge_includers = [edge_t, edge_b, edge_l, edge_r] | ||
| dummy_edge = np.zeros_like(edge_t, dtype=bool) # all-False so offset always applied | ||
| edge_includers.insert(0, dummy_edge) |
There was a problem hiding this comment.
| edge_t, edge_b = [coords[:,0] == lim for lim in (0, row_max)] | |
| edge_l, edge_r = [coords[:,1] == lim for lim in (0, col_max)] | |
| edge_includers = [edge_t, edge_b, edge_l, edge_r] | |
| dummy_edge = np.zeros_like(edge_t, dtype=bool) # all-False so offset always applied | |
| edge_includers.insert(0, dummy_edge) | |
| edge_includers = np.insert(np.equal(np.repeat(coords, 2, axis=1), | |
| [0, row_max, 0, col_max]), | |
| 0, False, axis=1) |
| If ``True``, use the fast SciPy function ``polygon_perimeter`` to locate | ||
| hull perimeter pixels and then fill it in. If ``False``, use the slower | ||
| Cython function ``grid_points_in_poly`` to locate convex hull pixels. | ||
| If ``None``, default to ``False`` (in future will default to ``True``). |
There was a problem hiding this comment.
We need to specify the version when the change will be effective.
Description
Bug fix for the patch submitted by @ehusby in #2928 3 years ago (which this PR supersedes), following advice from @jni to send in a fresh PR (as I don't have merge permissions so can't append to that PR as I previously tried to)
My contribution is to fix a bug in the application of offsets to the computed convex hull (specifically an
IndexErrorwhich arose when a half-pixel was rounded to a whole pixel and this whole pixel was one pixel outside of the perimeter of the image, and thus outside of the mask made from the image, which it tried to index into resulting in an error).Further to this, I have added in conditional checks so that this new routine is only used in the 2D case, as the previous PR would have applied to 3D cases too from what I can see (and I am under the impression it is explicitly only intended for 2D hulls, as the title of the PR stated).
While developing the modification I inspected the source of the bug (during and in the months following the 2020 SciPy sprint), in this repo, specifically the script
partial_coord_offset.pyreproduced the bug and then remediated it.Finally I ran the test suite in pytest, and it broke the majority of the convex hull-related tests and some others (detailed below).
Fixing the IndexError was the first step in @jni's to-do list from 2018, and upon reviewing this I have now also implemented the 2nd step (which involved re-inserting the code lines which Erik had originally removed, so that we get continuous performance and the pre-existing tests will still pass for now):
So this just leaves the tests to write for the new behaviour, and these can be handled to transition from the old routine to the new one by using the
fast_drawingparameter.Additionally, I will need to add the deprecation to the TODO list, I suggest:
Additionally, it should be deprecated by removal in v0.22
I haven't considered deprecation warnings yet, so will look into this (any guidance welcome but I expect it will be documented in
CONTRIBUTING.txtwhen I look more closely). Can someone confirm I got that right (about the TODO items and which sections they go in, and the need for deprecation warnings) please?Checklist
convex_hull_imagehas../doc/examples(already exists, as the feature is not new, just speeds up an existing feature)doc/examples/edges/plot_convex_hull.py./benchmarksskimage/morphology/tests/test_convex_hull.pyskimage/morphology/tests/test_convex_hull.py ........ [ 83%]skimage/morphology/tests/test_convex_hull.py F.FF.FFF [ 83%]Without deprecation:
With deprecation:
In conclusion, I need to next work on the tests to advance this PR to a stage at which it can be reviewed but want to submit at this point to initiate the process.
For reviewers
later.
__init__.py.doc/release/release_dev.rst.