Skip to content
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

add iterations option to binary morphology operations #5387

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented May 11, 2021

Description

Given that our binary morphology functions call the ones from SciPy, it would be useful to also expose arguments for iterations and origin. This PR adds an iterations keyword argument which just gets passed along to the underlying SciPy functions.

A motivating use case is to replace a single iteration of a large structuring element with multiple iterations of a smaller structuring element. For example

import numpy as np
from skimage import data, morphology

x = data.binary_blobs(1024)
morphology.binary_erosion(x, morphology.diamond(15))

vs.

morphology.binary_erosion(x, morphology.diamond(1), iterations=15)

The iterated case takes 10.8 ms vs. 139 ms for the larger diamond on my system.

Checklist

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.

replace use of numpy.testing and pytest calls with skimage._shared.testing
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @grlee77, great addition! We may be just need to choose the argument name iterations in the light of #4154. num_iter obtained some votes there.

BTW, I don't really like the aliases defined in skimage._shared.testing for numpy.testing and pytest functions. These functions are somehow standard, using these aliases can confuse contributors. But this is a detail 🙂

skimage/morphology/tests/test_binary.py Outdated Show resolved Hide resolved
@grlee77
Copy link
Contributor Author

grlee77 commented May 11, 2021

BTW, I don't really like the aliases defined in skimage._shared.testing for numpy.testing and pytest functions. These functions are somehow standard, using these aliases can confuse contributors. But this is a detail

I agree, but was thinking testing.parametrize was more commonly used here. A quick check reveals that this is mistaken, though:

(skimage_dev) lee8rx@lee8rx-MS-7B05:~/my_git/pyir/scikit-image/skimage$ grep -r "testing.parametrize"  | wc -l
67
(skimage_dev) lee8rx@lee8rx-MS-7B05:~/my_git/pyir/scikit-image/skimage$ grep -r "pytest.mark.parametrize"  | wc -l
175

so pytest is used directly about 70% of the time. I will go ahead and switch that back here...

@grlee77
Copy link
Contributor Author

grlee77 commented May 11, 2021

I am fine with using the assert_* from numpy.testing instead of skimage._shared.testing too, but am reluctant to make that change immediately because it will introduce a ton of conflicts with renaming of grey->gray in the same file in #5386

@grlee77
Copy link
Contributor Author

grlee77 commented May 12, 2021

Another motivation for adding this argument is my proposed structuring element decomposition feature (see #5388)

@rfezzani
Copy link
Member

@grlee77, I resolved the conflicts for this PR. Any update from your side?

@grlee77
Copy link
Contributor Author

grlee77 commented Oct 20, 2021

@grlee77, I resolved the conflicts for this PR. Any update from your side?

I think this issue is less high priority than I initially thought. I started this thinking I would use it to enable #5482, but I didn't end up needing it there (iterations are used, but only internally, not as a user-facing parameter). It may still be worth exposing the parameter to the end user, but then it does complicate things a little in the case of footprint sequences. One option would be to just multiply whatever iterations each sequence element already has by this "outer" number of iterations. What do you think?

@grlee77
Copy link
Contributor Author

grlee77 commented Oct 20, 2021

I think my preference would be to get #5482 merged first and then rebase this one afterwards. I can solicit a 2nd review in that PR.

@rfezzani
Copy link
Member

I agree, let's merge #5482 prior this PR and see then what is the best strategy to add the num_iter parameter 😉

@@ -70,6 +73,9 @@ def binary_dilation(image, footprint=None, out=None):
out : ndarray of bool, optional
The array to store the result of the morphology. If None is
passed, a new array will be allocated.
num_iter : int
The number of iterations to repeat binary_erosion with the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The number of iterations to repeat binary_erosion with the same
The number of iterations to repeat binary_dilation with the same

@@ -106,21 +113,24 @@ def binary_opening(image, footprint=None, out=None):
out : ndarray of bool, optional
The array to store the result of the morphology. If None
is passed, a new array will be allocated.
num_iter : int
The number of iterations to repeat binary_erosion with the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The number of iterations to repeat binary_erosion with the same
The number of iterations to repeat binary_opening with the same

@@ -141,13 +151,16 @@ def binary_closing(image, footprint=None, out=None):
out : ndarray of bool, optional
The array to store the result of the morphology. If None,
is passed, a new array will be allocated.
num_iter : int
The number of iterations to repeat binary_erosion with the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The number of iterations to repeat binary_erosion with the same
The number of iterations to repeat binary_closing with the same


@pytest.mark.parametrize(
"function",
["binary_erosion", "binary_dilation", "binary_closing", "binary_closing"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["binary_erosion", "binary_dilation", "binary_closing", "binary_closing"]
["binary_erosion", "binary_dilation", "binary_opening", "binary_closing"]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants