Add default level parameter in measure.find_contours #4862

rubywerman · 2020-07-28T18:28:48Z

Description

Fixes #4813
This PR adds the median as the default level parameter in find_contours. After reading the discussion in #4813 and looking at different image arrays [see notebook], I thought the median could be a good default value.

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

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.

pep8speaks · 2020-07-28T18:28:51Z

Hello @rubywerman! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-06 05:42:59 UTC

rfezzani · 2020-07-29T10:00:27Z

skimage/measure/_find_contours.py

@@ -9,7 +9,7 @@


 @deprecate_kwarg({'array': 'image'}, removed_version="0.20")
-def find_contours(image, level,
+def find_contours(image, level='median',


Setting the default to a string may suggest that other string values are accepted ('mean' for example)

Suggested change

def find_contours(image, level='median',

def find_contours(image, level=None,

rubywerman · 2020-07-29T15:44:11Z

Thank you for your feedback @rfezzani! I updated the parameter as you suggested :)

emmanuelle · 2020-07-29T16:19:02Z

Thanks for the PR @rubywerman! The discussion about which default level to choose was not completely settled in #4813, there is also the possibility in taking (max - min)/2. The latter choice is mostly useful for binary arrays while the median makes more sense for float arrays. I'd argue that binary arrays are the typical case where one does not want to specify the value of the level, because there is really only one value which makes sense. So I have a slight preference for (max - min)/2 but the median also makes sense.

Please also add a test which executes find_contours without passing the level value, so that the new API is tested.

rubywerman · 2020-07-29T19:28:36Z

Thanks for the feedback @emmanuelle! I will add the test. Also, could it be beneficial to have two default values, (max - min) / 2 and median, depending on whether it's a binary or float array?

jni · 2020-07-30T06:22:45Z

I agree with @sciunto that having different behaviour based on the input type isn't ideal. I have commented on the original issue, but my suggestion would be to have np.mean(np.quantile(image, (0.005, 0.995))) as the default. Thoughts?

emmanuelle · 2020-07-31T10:14:23Z

I have reconsidered my initial doubts. Given the fact that there is the possibility to pass a value, and that for a binary image other contours than 0.5 (in fact anything strictly between 0 and 1) will also good give results, we have to accept that there are several possible good solutions and just choose one of them. The one you suggest @jni is a good one, in fact the original median value proposed by @rubywerman is also good (and simple to explain).

emmanuelle · 2020-08-04T14:41:33Z

skimage/measure/_find_contours.py

@@ -24,7 +24,8 @@ def find_contours(image, level,
    image : 2D ndarray of double
        Input image in which to find contours.
    level : float
-        Value along which to find contours in the array.
+        Value along which to find contours in the array. By default, the level
+        is set to the median value of the array.


please also change the docstring :-)

emmanuelle · 2020-08-04T14:44:01Z

skimage/measure/tests/test_find_contours.py

@@ -130,3 +130,40 @@ def test_invalid_input():
        find_contours(r, 0.5, 'foo', 'bar')
    with testing.raises(ValueError):
        find_contours(r[..., None], 0.5)
+


thanks for adding all these tests :-)

emmanuelle

Thanks @rubywerman ! I think the PR is ready now.

skimage/measure/_find_contours.py

sciunto · 2020-08-06T05:07:12Z

skimage/morphology/convex_hull.py

-    if image.ndim > 2:
-        raise ValueError("Input must be a 2D image")
+    if image.ndim > 3:
+        raise ValueError("Input must be a 2D or 3D image")


Why this change in this PR?

my mistake– it shouldn't. fixing now

I thought @rubywerman was fixing #4865 at the same time :-) (but it can be in another PR, with a test ;-))

sciunto · 2020-08-06T05:44:31Z

I approve it as well, I'm just waiting for the CI. Thank you for your contribution.

emmanuelle · 2020-08-06T07:55:24Z

Merging, thank you @rubywerman !

rubywerman added 3 commits July 28, 2020 12:47

add default level

8887435

add note

76630bd

remove trailing whitespace

da972d9

rubywerman added 2 commits July 28, 2020 13:30

fix style

dd099b3

change divison to nump opp

a7e6cd3

rubywerman changed the title ~~Add default level parameter in measure.find_contours~~ [WIP] Add default level parameter in measure.find_contours Jul 28, 2020

rfezzani reviewed Jul 29, 2020

View reviewed changes

change default from string

681b3d0

change level to max/min suggestion and add tests

00eb715

emmanuelle reviewed Aug 4, 2020

View reviewed changes

update docstring

a241136

emmanuelle changed the title ~~[WIP] Add default level parameter in measure.find_contours~~ Add default level parameter in measure.find_contours Aug 5, 2020

emmanuelle approved these changes Aug 5, 2020

View reviewed changes

sciunto reviewed Aug 6, 2020

View reviewed changes

skimage/measure/_find_contours.py Outdated Show resolved Hide resolved

Update skimage/measure/_find_contours.py

f5b541b

sciunto reviewed Aug 6, 2020

View reviewed changes

fix erroneous commit

3444b89

sciunto approved these changes Aug 6, 2020

View reviewed changes

emmanuelle merged commit 81d0ccb into scikit-image:master Aug 6, 2020

sciunto mentioned this pull request Aug 10, 2020

2020's calendar of community management #4486

Closed

jni mentioned this pull request Nov 27, 2020

Fix default level computation for find_contours #5083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default level parameter in measure.find_contours #4862

Add default level parameter in measure.find_contours #4862

rubywerman commented Jul 28, 2020 •

edited

Loading

pep8speaks commented Jul 28, 2020 •

edited

Loading

rfezzani Jul 29, 2020

rubywerman commented Jul 29, 2020

emmanuelle commented Jul 29, 2020

rubywerman commented Jul 29, 2020 •

edited

Loading

jni commented Jul 30, 2020

emmanuelle commented Jul 31, 2020

emmanuelle Aug 4, 2020

emmanuelle Aug 4, 2020

emmanuelle left a comment

sciunto Aug 6, 2020

rubywerman Aug 6, 2020

sciunto Aug 6, 2020

emmanuelle Aug 6, 2020

sciunto commented Aug 6, 2020

emmanuelle commented Aug 6, 2020

	def find_contours(image, level='median',
	def find_contours(image, level=None,

Add default level parameter in measure.find_contours #4862

Add default level parameter in measure.find_contours #4862

Conversation

rubywerman commented Jul 28, 2020 • edited Loading

Description

Checklist

For reviewers

pep8speaks commented Jul 28, 2020 • edited Loading

Comment last updated at 2020-08-06 05:42:59 UTC

rfezzani Jul 29, 2020

Choose a reason for hiding this comment

rubywerman commented Jul 29, 2020

emmanuelle commented Jul 29, 2020

rubywerman commented Jul 29, 2020 • edited Loading

jni commented Jul 30, 2020

emmanuelle commented Jul 31, 2020

emmanuelle Aug 4, 2020

Choose a reason for hiding this comment

emmanuelle Aug 4, 2020

Choose a reason for hiding this comment

emmanuelle left a comment

Choose a reason for hiding this comment

sciunto Aug 6, 2020

Choose a reason for hiding this comment

rubywerman Aug 6, 2020

Choose a reason for hiding this comment

sciunto Aug 6, 2020

Choose a reason for hiding this comment

emmanuelle Aug 6, 2020

Choose a reason for hiding this comment

sciunto commented Aug 6, 2020

emmanuelle commented Aug 6, 2020

rubywerman commented Jul 28, 2020 •

edited

Loading

pep8speaks commented Jul 28, 2020 •

edited

Loading

rubywerman commented Jul 29, 2020 •

edited

Loading