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

FIX: select num_peaks if labels is specified #2098

Merged
merged 5 commits into from Sep 6, 2016

Conversation

Projects
None yet
9 participants
@sciunto
Member

sciunto commented May 21, 2016

This PR fixes the bug described in #1726.

  • I moved duplicated code into an internal function
  • I added a unittest (was not covered before).
Show outdated Hide outdated skimage/feature/tests/test_peak.py
assert len(peak.peak_local_max(image, min_distance=1, threshold_abs=0)) == 5
peaks_limited = peak.peak_local_max(
image, min_distance=1, threshold_abs=0, num_peaks=2)
assert len(peaks_limited) == 2

This comment has been minimized.

@soupault

soupault May 21, 2016

Member

This test does not seem to address the original issue. To cover the case, we need at least 2 labels for input image and call peak_local_max with labels parameter.

@soupault

soupault May 21, 2016

Member

This test does not seem to address the original issue. To cover the case, we need at least 2 labels for input image and call peak_local_max with labels parameter.

This comment has been minimized.

@sciunto

sciunto May 22, 2016

Member

it's a typo

@sciunto

sciunto May 22, 2016

Member

it's a typo

@soupault

This comment has been minimized.

Show comment
Hide comment
@soupault

soupault May 21, 2016

Member

@mskoh52 @sciunto what should be the preferable behaviour: to return <= num_peaks total, or <= num_peaks for each label?

Member

soupault commented May 21, 2016

@mskoh52 @sciunto what should be the preferable behaviour: to return <= num_peaks total, or <= num_peaks for each label?

@mskoh52

This comment has been minimized.

Show comment
Hide comment
@mskoh52

mskoh52 May 21, 2016

I don't think there is a "right answer", but I would vote for <= num_peaks for each label. I'm imagining partitioning an image into regions of interest with the labels and trying to find peaks in each one, so in that case I'd want to treat each ROI independently.

mskoh52 commented May 21, 2016

I don't think there is a "right answer", but I would vote for <= num_peaks for each label. I'm imagining partitioning an image into regions of interest with the labels and trying to find peaks in each one, so in that case I'd want to treat each ROI independently.

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto May 22, 2016

Member

We can add another option like num_peaks_per_label ?

Right now, if num_peaks is for each label, it's not compatible with the actual docstring.

Member

sciunto commented May 22, 2016

We can add another option like num_peaks_per_label ?

Right now, if num_peaks is for each label, it's not compatible with the actual docstring.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 22, 2016

Current coverage is 90.67% (diff: 100%)

Merging #2098 into master will increase coverage by 0.01%

@@             master      #2098   diff @@
==========================================
  Files           303        303          
  Lines         21763      21797    +34   
  Methods           0          0          
  Messages          0          0          
  Branches       2012       2012          
==========================================
+ Hits          19731      19765    +34   
  Misses         1669       1669          
  Partials        363        363          

Powered by Codecov. Last update e691467...2486759

codecov-io commented May 22, 2016

Current coverage is 90.67% (diff: 100%)

Merging #2098 into master will increase coverage by 0.01%

@@             master      #2098   diff @@
==========================================
  Files           303        303          
  Lines         21763      21797    +34   
  Methods           0          0          
  Messages          0          0          
  Branches       2012       2012          
==========================================
+ Hits          19731      19765    +34   
  Misses         1669       1669          
  Partials        363        363          

Powered by Codecov. Last update e691467...2486759

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto May 22, 2016

Member

@mskoh52 @soupault I added that option + unittests

Member

sciunto commented May 22, 2016

@mskoh52 @soupault I added that option + unittests

Show outdated Hide outdated skimage/feature/tests/test_peak.py
def test_num_peaks_per_labels():
image = np.zeros((7, 7), dtype=np.uint8)
labels = np.zeros((7, 7), dtype=np.uint8) + 1 #0*6.5

This comment has been minimized.

@soupault

soupault May 25, 2016

Member

What does the comment mean?

@soupault

soupault May 25, 2016

Member

What does the comment mean?

This comment has been minimized.

@sciunto

sciunto May 25, 2016

Member

oops.

@sciunto

sciunto May 25, 2016

Member

oops.

Show outdated Hide outdated skimage/feature/tests/test_peak.py
image, min_distance=1, threshold_abs=0, labels=labels, num_peaks=2)
assert len(peaks_limited) == 2
def test_num_peaks_per_labels():

This comment has been minimized.

@soupault

soupault May 25, 2016

Member

I'm not sure about this test.
On the one hand, it doesn't check if num_peaks and num_peaks_per_label are different (the former is checked in the test above with the same test case). On the other hand, it doesn't check anything additional to test_num_peaks_and_labels_4quadrants.
I think you should merge test_num_peaks_and_labels and test_num_peaks_per_labels: for example, create image where the same values of num_peaks and num_peaks_per_label guarantee different results and put all 3 asserts into single test.

@soupault

soupault May 25, 2016

Member

I'm not sure about this test.
On the one hand, it doesn't check if num_peaks and num_peaks_per_label are different (the former is checked in the test above with the same test case). On the other hand, it doesn't check anything additional to test_num_peaks_and_labels_4quadrants.
I think you should merge test_num_peaks_and_labels and test_num_peaks_per_labels: for example, create image where the same values of num_peaks and num_peaks_per_label guarantee different results and put all 3 asserts into single test.

Show outdated Hide outdated skimage/feature/tests/test_peak.py
def test_num_peaks_and_labels_4quadrants():
image = np.random.uniform(size=(40, 60))

This comment has been minimized.

@soupault

soupault May 25, 2016

Member

Please, specify the seed. We may face assertion fail from time to time, if /dev/random has bad luck.

The test itself is OK, but I'd consider decreasing the image size. Let's say to 20x20.

@soupault

soupault May 25, 2016

Member

Please, specify the seed. We may face assertion fail from time to time, if /dev/random has bad luck.

The test itself is OK, but I'd consider decreasing the image size. Let's say to 20x20.

This comment has been minimized.

@sciunto

sciunto May 25, 2016

Member

There is a seed at the top of the file.

@sciunto

sciunto May 25, 2016

Member

There is a seed at the top of the file.

This comment has been minimized.

@sciunto

sciunto May 25, 2016

Member

I decreased the img size

@sciunto

sciunto May 25, 2016

Member

I decreased the img size

@soupault

This comment has been minimized.

Show comment
Hide comment
@soupault

soupault May 25, 2016

Member

@sciunto Sorry for being so meticulous :). Several comments more.

This recursive code is fun! Hope we'll refactor it one day.

Member

soupault commented May 25, 2016

@sciunto Sorry for being so meticulous :). Several comments more.

This recursive code is fun! Hope we'll refactor it one day.

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto May 25, 2016

Member

no problem ;) It's better to be meticulous instead of blowing up the house after the next release :)

Member

sciunto commented May 25, 2016

no problem ;) It's better to be meticulous instead of blowing up the house after the next release :)

Show outdated Hide outdated skimage/feature/tests/test_peak.py
image = np.random.uniform(size=(40, 60))
i, j = np.mgrid[0:40, 0:60]
labels = 1 + (i >= 20) + (j >= 30) * 2
image = np.random.uniform(size=(20, 30))

This comment has been minimized.

This comment has been minimized.

@sciunto

sciunto May 27, 2016

Member

It's defined globally at line 8, should be enough no?

@sciunto

sciunto May 27, 2016

Member

It's defined globally at line 8, should be enough no?

This comment has been minimized.

@stefanv

stefanv May 27, 2016

Member

I guess that should be fine?

On Fri, 27 May 2016 at 11:02 François Boulogne notifications@github.com
wrote:

In skimage/feature/tests/test_peak.py
#2098 (comment)
:

@@ -272,14 +312,14 @@ def test_adjacent_different_objects():

def test_four_quadrants():

  • image = np.random.uniform(size=(40, 60))
  • i, j = np.mgrid[0:40, 0:60]
  • labels = 1 + (i >= 20) + (j >= 30) * 2
  • image = np.random.uniform(size=(20, 30))

It's defined globally at line 8, should be enough no?


You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub
https://github.com/scikit-image/scikit-image/pull/2098/files/b84af59c301beb2ce04639d22b4227b229c94493#r64944523,
or mute the thread
https://github.com/notifications/unsubscribe/AACwD6yqFD9XysFbFgA1XTY1NqgjQWRSks5qFzGlgaJpZM4Ijx7J
.

@stefanv

stefanv May 27, 2016

Member

I guess that should be fine?

On Fri, 27 May 2016 at 11:02 François Boulogne notifications@github.com
wrote:

In skimage/feature/tests/test_peak.py
#2098 (comment)
:

@@ -272,14 +312,14 @@ def test_adjacent_different_objects():

def test_four_quadrants():

  • image = np.random.uniform(size=(40, 60))
  • i, j = np.mgrid[0:40, 0:60]
  • labels = 1 + (i >= 20) + (j >= 30) * 2
  • image = np.random.uniform(size=(20, 30))

It's defined globally at line 8, should be enough no?


You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub
https://github.com/scikit-image/scikit-image/pull/2098/files/b84af59c301beb2ce04639d22b4227b229c94493#r64944523,
or mute the thread
https://github.com/notifications/unsubscribe/AACwD6yqFD9XysFbFgA1XTY1NqgjQWRSks5qFzGlgaJpZM4Ijx7J
.

This comment has been minimized.

@sciunto

sciunto May 28, 2016

Member

From @stefanv comment, I saw that in __init__.py, we call setup_test() from _shared.testing. So, we have a seed there. Line 8 is redundant.

@sciunto

sciunto May 28, 2016

Member

From @stefanv comment, I saw that in __init__.py, we call setup_test() from _shared.testing. So, we have a seed there. Line 8 is redundant.

This comment has been minimized.

@soupault

soupault May 28, 2016

Member

What is we need a seed different from the one defined in that setup_test()?
I personally prefer to initialize the RNG in the very same place where it is used.

@soupault

soupault May 28, 2016

Member

What is we need a seed different from the one defined in that setup_test()?
I personally prefer to initialize the RNG in the very same place where it is used.

This comment has been minimized.

@sciunto

sciunto May 28, 2016

Member

OK, I put a seed at the top of each function with random calls. I think that we'll need to setup our tests with a fixture as Stefan said in #2113, but that's another tasks (it impacts several test suites). Does it make sense?

@sciunto

sciunto May 28, 2016

Member

OK, I put a seed at the top of each function with random calls. I think that we'll need to setup our tests with a fixture as Stefan said in #2113, but that's another tasks (it impacts several test suites). Does it make sense?

This comment has been minimized.

@stefanv

stefanv Jun 2, 2016

Member
@stefanv

stefanv via email Jun 2, 2016

Member

This comment has been minimized.

@blink1073

blink1073 Jun 2, 2016

Member

Why, because nose is no longer supported ;)?

@blink1073

blink1073 Jun 2, 2016

Member

Why, because nose is no longer supported ;)?

This comment has been minimized.

@blink1073

blink1073 Jun 2, 2016

Member

It is not a small task for a larger lib, see: matplotlib/matplotlib#5405

@blink1073

blink1073 Jun 2, 2016

Member

It is not a small task for a larger lib, see: matplotlib/matplotlib#5405

@soupault

This comment has been minimized.

Show comment
Hide comment
@soupault

soupault Jun 2, 2016

Member

👍

Member

soupault commented Jun 2, 2016

👍

@sciunto sciunto closed this Jun 11, 2016

@sciunto sciunto reopened this Jun 11, 2016

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Jun 11, 2016

Member

@scikit-image/core Anything else for this PR?

Member

sciunto commented Jun 11, 2016

@scikit-image/core Anything else for this PR?

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Jun 21, 2016

Member

@soupault Do we need another opinion here?

Member

sciunto commented Jun 21, 2016

@soupault Do we need another opinion here?

@soupault

This comment has been minimized.

Show comment
Hide comment
@soupault

soupault Jun 21, 2016

Member

@sciunto we do. According to the policy one isn't permitted to vote/merge his/her own PR.

Member

soupault commented Jun 21, 2016

@sciunto we do. According to the policy one isn't permitted to vote/merge his/her own PR.

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Aug 1, 2016

Member

@jni @ahojnnes Are you available to review this PR please? It's pending for a while. ;)

Member

sciunto commented Aug 1, 2016

@jni @ahojnnes Are you available to review this PR please? It's pending for a while. ;)

Show outdated Hide outdated skimage/feature/peak.py
@@ -41,6 +42,8 @@ def peak_local_max(image, min_distance=1, threshold_abs=None,
num_peaks : int, optional
Maximum number of peaks. When the number of peaks exceeds `num_peaks`,
return `num_peaks` peaks based on highest peak intensity.
num_peaks_per_label : int, optional
Maximum number of peaks for each label.

This comment has been minimized.

@jni

jni Aug 2, 2016

Contributor

This is an API breaking change (until we switch to Python3 only and make these keyword-only arguments). I suggest you put this new argument at the end so we can avoid the deprecation.

@jni

jni Aug 2, 2016

Contributor

This is an API breaking change (until we switch to Python3 only and make these keyword-only arguments). I suggest you put this new argument at the end so we can avoid the deprecation.

This comment has been minimized.

@sciunto

sciunto Aug 2, 2016

Member

Good point. I hesitated because it looks less natural, but you're right. We can change that later... As time goes, I'm more and more favourable to move to py3 only (I was not clear even few months ago).

@sciunto

sciunto Aug 2, 2016

Member

Good point. I hesitated because it looks less natural, but you're right. We can change that later... As time goes, I'm more and more favourable to move to py3 only (I was not clear even few months ago).

Show outdated Hide outdated skimage/feature/peak.py
image.shape)]
idx_maxsort = np.argsort(intensities)[::-1]
coord = coord[idx_maxsort][:num_peaks]
return coord

This comment has been minimized.

@jni

jni Aug 2, 2016

Contributor

Unless I'm missing something, there is no reason for this function to be nested? Pull it out and add a leading _ underscore to indicate that it's a private function.

@jni

jni Aug 2, 2016

Contributor

Unless I'm missing something, there is no reason for this function to be nested? Pull it out and add a leading _ underscore to indicate that it's a private function.

This comment has been minimized.

@jni

jni Aug 2, 2016

Contributor

Also, you can save a bit of work by transposing later:

coord = np.nonzero(mask)
if len(coord[0]) > num_peaks:
    intensities = image[coord]
    idx_maxsort = np.argsort(intensities)
    coord = np.transpose(coord)[idx_maxsort, -num_peaks:]
return coord
@jni

jni Aug 2, 2016

Contributor

Also, you can save a bit of work by transposing later:

coord = np.nonzero(mask)
if len(coord[0]) > num_peaks:
    intensities = image[coord]
    idx_maxsort = np.argsort(intensities)
    coord = np.transpose(coord)[idx_maxsort, -num_peaks:]
return coord

This comment has been minimized.

@sciunto

sciunto Aug 2, 2016

Member

This code doesn't work... :)

@sciunto

sciunto Aug 2, 2016

Member

This code doesn't work... :)

This comment has been minimized.

@jni

jni Aug 3, 2016

Contributor

Well, I just saw that my indexing is wrong, should be [idx_maxsort][-num_peaks:], but other than that it should work...?

@jni

jni Aug 3, 2016

Contributor

Well, I just saw that my indexing is wrong, should be [idx_maxsort][-num_peaks:], but other than that it should work...?

This comment has been minimized.

@sciunto

sciunto Aug 3, 2016

Member

A correct function would be:

    coord = np.nonzero(mask)
    if len(coord[0]) > num_peaks:
        intensities = image[coord]
        idx_maxsort = np.argsort(intensities)
        coord = np.transpose(coord)[idx_maxsort][-num_peaks:]
        coord = np.row_stack(coord)
    else:
        coord = np.column_stack(coord)
    return coord
@sciunto

sciunto Aug 3, 2016

Member

A correct function would be:

    coord = np.nonzero(mask)
    if len(coord[0]) > num_peaks:
        intensities = image[coord]
        idx_maxsort = np.argsort(intensities)
        coord = np.transpose(coord)[idx_maxsort][-num_peaks:]
        coord = np.row_stack(coord)
    else:
        coord = np.column_stack(coord)
    return coord

This comment has been minimized.

@jni

jni Aug 4, 2016

Contributor

You shouldn't need the row_stack call?

btw I was completely unaware of row_stack and column_stack! So handy! I can't tell you how many times I've used newaxis with hstack, which is super-annoying!

At any rate, here's some nice middle ground, seeing the issue of having to stack one way or another:

coord = np.transpose(np.nonzero(mask))
if coord.shape[0] > num_peaks:
    intensities = image[tuple(coord.T)]
    top_idxs = np.argsort(intensities)[-num_peaks:]
    coord = coord[top_idxs]
@jni

jni Aug 4, 2016

Contributor

You shouldn't need the row_stack call?

btw I was completely unaware of row_stack and column_stack! So handy! I can't tell you how many times I've used newaxis with hstack, which is super-annoying!

At any rate, here's some nice middle ground, seeing the issue of having to stack one way or another:

coord = np.transpose(np.nonzero(mask))
if coord.shape[0] > num_peaks:
    intensities = image[tuple(coord.T)]
    top_idxs = np.argsort(intensities)[-num_peaks:]
    coord = coord[top_idxs]

This comment has been minimized.

@sciunto

sciunto Aug 4, 2016

Member

I'm positive to say that if you remove the row_stack, tests blow up. I do not remember exactly the different shapes, but for sure, you need it. :)

I'm not sure which writing is the best (ie also the most legible).

@sciunto

sciunto Aug 4, 2016

Member

I'm positive to say that if you remove the row_stack, tests blow up. I do not remember exactly the different shapes, but for sure, you need it. :)

I'm not sure which writing is the best (ie also the most legible).

This comment has been minimized.

@jni

jni Aug 5, 2016

Contributor

I'm positive that removing ravel_multi_index and flatten is a readability win. =)

@jni

jni Aug 5, 2016

Contributor

I'm positive that removing ravel_multi_index and flatten is a readability win. =)

This comment has been minimized.

@jni

jni Aug 5, 2016

Contributor

*flat

@jni

jni Aug 5, 2016

Contributor

*flat

for imin, imax in ((0, 20), (20, 40)):
for jmin, jmax in ((0, 30), (30, 60)):
for imin, imax in ((0, 10), (10, 20)):
for jmin, jmax in ((0, 15), (15, 30)):

This comment has been minimized.

@jni

jni Aug 2, 2016

Contributor

Why did this test change?

@jni

jni Aug 2, 2016

Contributor

Why did this test change?

This comment has been minimized.

@sciunto

sciunto Aug 2, 2016

Member

I made the size smaller, see above (just to run the tests faster.. a little)

@sciunto

sciunto Aug 2, 2016

Member

I made the size smaller, see above (just to run the tests faster.. a little)

@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Aug 2, 2016

Contributor

@sciunto I have some concerns... =P

Contributor

jni commented Aug 2, 2016

@sciunto I have some concerns... =P

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Aug 5, 2016

Member

@jni Your version is pushed. :)

Member

sciunto commented Aug 5, 2016

@jni Your version is pushed. :)

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Sep 5, 2016

Member

This PR felt in the forgotten PR basket :) Anyone for a second +1?

Member

sciunto commented Sep 5, 2016

This PR felt in the forgotten PR basket :) Anyone for a second +1?

@ahojnnes

This comment has been minimized.

Show comment
Hide comment
@ahojnnes

ahojnnes Sep 5, 2016

Member

LGTM, but would like to hear @jni as well.

Member

ahojnnes commented Sep 5, 2016

LGTM, but would like to hear @jni as well.

@emmanuelle

This comment has been minimized.

Show comment
Hide comment
@emmanuelle

emmanuelle Sep 5, 2016

Member

@jni is back from holidays I think :-)

Member

emmanuelle commented Sep 5, 2016

@jni is back from holidays I think :-)

@jni

This comment has been minimized.

Show comment
Hide comment
@jni

jni Sep 6, 2016

Contributor

@emmanuelle you're right! =) Looking now. =)

Contributor

jni commented Sep 6, 2016

@emmanuelle you're right! =) Looking now. =)

@jni jni merged commit 92a3851 into scikit-image:master Sep 6, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sciunto sciunto deleted the sciunto:bug_localmaxpeak branch Sep 6, 2016

@sciunto

This comment has been minimized.

Show comment
Hide comment
@sciunto

sciunto Sep 6, 2016

Member

thanks @jni

Member

sciunto commented Sep 6, 2016

thanks @jni

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment