WIP: Refactor to skimage.morphology.footprint_ellipse#7628
WIP: Refactor to skimage.morphology.footprint_ellipse#7628lagru wants to merge 11 commits intoscikit-image:mainfrom
skimage.morphology.footprint_ellipse#7628Conversation
This attempts to replace `disk` and `ball` internally while preserving their original behavior. Unfortunately, this appears to be more tricky than anticipated, because of `strict_radius`, only partial support for ND and yet to be investigated edge cases.
If we keep this deprecated but around for a while it's probably a good idea to revert to the original algorithm. I intend to cover the backwards-compability of the new algo `footprint_ellipse` in a full test but if there are edge cases we miss, this should keep users who stay on the old version insulated.
If we keep this deprecated but around for a while it's probably a good idea to revert to the original algorithm. I intend to cover the backwards-compability of the new algo `footprint_ellipse` in a full test but if there are edge cases we miss, this should keep users who stay on the old version insulated.
For `radius=1` should be fully compatible with the previous behavior and not only approximately so (there are some float rounding errors that lead to small differences for large radii when comparing `ball` and `disk` with `footprint_ellipse`.
`ball` and `ellipse` differ in whether the border =1 of the ellipse equation is included inside the footprint or not. `ellipse` doesn't include the border, while `ball` and `disk` do. Adding a parameter `border` to control this behavior, allows `footprint_ellipse` to approximate the behavior of previous deprecated algorithms.
Unfortunately, there seem to be slight differences when trying to reproduce `disk` and `ball` shaped footprints with `footprint_ellipse`. Right now I'm guessing that this might be due to floating point calculations. `disk` and `ball` construct the equation in a different way, than `footprint_ellipse` which supports N dimensions. Pushing this now to facilitate discussion.
| def test_footprint_ball_backwards_compatibility(radius): | ||
| with pytest.warns(FutureWarning, match="`ball` is deprecated"): | ||
| old = ski.morphology.ball(radius) | ||
| new = ski.morphology.footprint_ellipse(shape=(radius * 2 + 1,) * 3, grow=0.0001) |
There was a problem hiding this comment.
Note that grow=0.0001 is necessary to pass this test for radius=33. From 220f453
Unfortunately, there seem to be slight differences when trying to reproduce
diskandballshaped footprints withfootprint_ellipse. Right now I'm guessing that this might be due to floating point calculations.
diskandballconstruct the equation in a different way, thanfootprint_ellipsewhich supports N dimensions.
Since we deprecate but don't plan on removing ball, disk and ellipse until skimage2, I think this is fine. Though, ideally, we can figure out why this small correction is necessary. 🤔
skimage/morphology/footprints.py
Outdated
| decomposition : {None, 'sequence', 'crosses'}, optional | ||
| TBD |
There was a problem hiding this comment.
Right now I am undecided how to handle the decomposition. If I understand the documentation correctly, then decomposition strategies do not return the same footprint as the composed one. It's actually a bunch of different algorithms that might lead to slightly different results.
Another complicating factor in case in context of footprint_ellipse is, that sequence decomposition is only supported for spherical footprints.
I think all this combines to a very confusing and hard to reason about API. It's also hard to document and test.
One solution could be to separate the API into
def footprint_ellipse(shape, grow=, border=, dtype=)
# Supports 2D, 3D
def footprint_disk_decomposed(shape, dtype=)
# Formerly private, makes it clear that a
# composed footprint is constructed before decomposition
# in contrast to `footprint_disk_decomposed`
def cross_decomposition(footprint)
# deprecated
def disk(radius, dtype=, *, strict_radius=, decomposition=)
def ball(radius, dtype=, *, strict_radius=, decomposition=)
def ellipse(width, height, dtype=, *, decomposition=)But that also doesn't feel terribly perfect from a user side perhaps? Thoughts?
| adjust_radii : float or sequence of floats, optional | ||
| Adjust ellipse size within the footprint. Positive values will increase | ||
| the axes of the ellipse. With 0, the border of the ellipsis will touch | ||
| the border of the generated footprint. Ignored, if `decomposition` is | ||
| not `None`. |
There was a problem hiding this comment.
Hello! I just wanted to bring attention to the fact that this parameter would solve the following issue:
(Thanks much for including it in your implementation!)
Description
This is a WIP exploring the problem space. I'm attempting to replace
diskandballinternally while preserving their original behavior. Unfortunately, this appears to be more tricky than anticipated, because ofstrict_radius, only partial support for ND and yet to be investigated edge cases.It seems replacing
ellipsewill be even harder. That usesdraw.ellipseinternally which appears to generate ellipses that are different from the current approach. The current approach usesx²/a² + y²/b² <= 1to determine which pixels are part of the ellipse / ellipsoid.Checklist
./doc/examplesfor new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.