Skip to content

Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix#3546

Merged
jni merged 34 commits intoscikit-image:v0.14.xfrom
lagru:backport_v0.14.x/3022_3447
Nov 14, 2018
Merged

Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix#3546
jni merged 34 commits intoscikit-image:v0.14.xfrom
lagru:backport_v0.14.x/3022_3447

Conversation

@lagru
Copy link
Copy Markdown
Member

@lagru lagru commented Nov 14, 2018

Description

Backports changes from #3022 and #3447 onto the current v0.14.x branch.

Checklist

References

See also #3545.

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@stefanv, @jni

lagru added 30 commits November 14, 2018 10:18
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]
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.
lagru and others added 4 commits November 14, 2018 10:18
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
@pep8speaks
Copy link
Copy Markdown

Hello @lagru! Thanks for submitting the PR.

Line 150:16: W504 line break after binary operator

@lagru
Copy link
Copy Markdown
Member Author

lagru commented Nov 14, 2018

The test failure is caused by a failing doctest for skimage.util.lookfor.lookfor. #3543, #3477 and others give me the feeling that this is a recurring and known issue... 😅

@jni
Copy link
Copy Markdown
Member

jni commented Nov 14, 2018

Thanks @lagru! Brilliant! =) I'm gonna go ahead and merge this. We can look for the lookfor fix later. =P

@jni jni merged commit d2a05a0 into scikit-image:v0.14.x Nov 14, 2018
@lagru lagru deleted the backport_v0.14.x/3022_3447 branch April 18, 2020 16:36
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.

3 participants