Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix#3546
Merged
jni merged 34 commits intoscikit-image:v0.14.xfrom Nov 14, 2018
Merged
Conversation
These two new functions work very similar to skimage.morphology.local_maxima but are generally faster.
Maxima touching the image border are included by simply padding the array with its minimal value. Thus a maximum is always at least 1 sample away from the image border and will not be omitted. Furthermore the user can pass in a custom structured element himself with the modified argument `selem` (former `connectivity`). [ci skip]
[ci skip]
The test failure in TestExtrema.test_local_extrema_uniform at test_extrema.py:214 is expected for dtypes np.uint64 & np.int64. This happens because numpy.pad doesn't correctly pad the inverted image. This seems to be a bug in NumPy (see numpy/numpy#11027).
This replaces the slower solution using pad / crop with a function that directly sets the edge values.
Furthermore this avoids the bug in numpy.pad for large integers (see numpy/numpy#11027) which means unit tests are expected to pass now.
Other: Add validation of selem if given as array.
Note to removal of TODO in _restorable_queue.pxi: I think checking for an overflow of Py_ssize_t isn't necessary in this case. The allocation of additional memory would be problematic long before the overflow would be relevant.
Prefiltering in the last dimension based on sample comparison only makes sense if these are part of the structuring element (-1 and 1 in neighbor_offsets). If this is not the case, simply skip this step and evaluate every sample in the image.
Some parts of the old test were moved to the new test class.
and add appropriate tests.
Unfortunately Cython doesn't seem to support the import of variables declared in the global space of the PYX file.
These two functions essentially do the same thing. Choose the more descriptive names and documentation from the new function but keep the sorting and the option to specify a custom center in `selem`. The reasoning behind supplying `image_shape` instead of `image` is that input arguments should be as simple as possible.
Reasoning: peak_local_max's argument exclude_border isn't a boolean switch but an integer that denotes how much of the border is excluded. So using a similar name might actually be more confusing than useful in this case.
Deal with special case of np.float16 which has not equivalent _t type gracefully by raising a more descriptive exception.
which is what _local_maxima expects image to be.
- Don't break API: argument `selem` must be second argument to be backwards compatible. - Don't modify input in _offset_to_raveled_neighbors. - Use `structure` instead of `selem` (where possible) which is closer to SciPy's terminology.
Don't use NOT_MAXIMUM for boolean flag `is_maximum`. This might break if the value is changed and no longer 0.
Failed due to mismatching display formats on different platforms. This should alleviate the issue.
…-image#3447) * Fix local_maxima for input with dimensions < 3 local_maxima (and local_minima) failed with an ValueError for images with at least 1 dimension with fewer than 3 elements. This was due to the implementation which applies a structuring element with exactly 3 elements in each dimension to the image in question. This behavior was only visible if `allow_borders` was false, otherwise the image was padded internally. This fix makes these functions work for the mentioned edge cases by testing the image shape at the appropriate time. * Fix output of local_maxima for empty arrays The old output for empty arrays if `indices` was true didn't correspond to the output for non-empty arrays and the description in the docstring. This ensures that the output will always be a tuple of empty arrays. If `indices` is false, the returned empty array now machtes the shape of `image` as well. * Show warning if no maxima are possible which is the case for dimensions with fewer than 3 samples. Because `local_minima` wraps `local_maxima` the warning can only set the correct stacklevel for one of these functions. The latter was chosen because it stands to reason that it is / will be used more frequently. If may be possible to suppress and re-raise the warning with the context manager from the warnings module but that would introduce non-thread- safe code and introduce perhaps unnecessary complexity. * Point user to allow_borders in warning message
|
Hello @lagru! Thanks for submitting the PR.
|
Member
Author
Member
|
Thanks @lagru! Brilliant! =) I'm gonna go ahead and merge this. We can look for the lookfor fix later. =P |
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Backports changes from #3022 and #3447 onto the current
v0.14.xbranch.Checklist
Gallery example in./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
References
See also #3545.
For reviewers
later.
__init__.py.doc/release/release_dev.rst.Consider backporting the PR with@meeseeksdev backport to v0.14.x@stefanv, @jni