Implemented Kadir and Brady's Saliency detection #2299

parulsethi · 2016-09-09T14:27:01Z

Description

This PR implements Kadir and Brady's Saliency detection algorithm which calculates the saliency score by measuring the entropy of the region's descriptor(to find locally complex regions) and weigh them by the difference over multiple scales to select features that are globally discriminative.
Result over astronaut():

As this is my first contribution to not only scikit-image but open-source too, so please suggest the changes that are required in order to merge this, if you are interested in adding this feature.
I haven't yet prepared the doc example as their are some parameters in this algorithm over which i need your suggestion that whether to leave them as optional or set it to the optimized one. These are the saliency and clustering thresholds.

Checklist

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Unit tests

References

Kadir, Timor, and Michael Brady. "Saliency, scale and image description." International Journal of Computer Vision 45.2 (2001): 83-105.

codecov-io · 2016-09-09T16:16:02Z

Current coverage is 90.68% (diff: 100%)

Merging #2299 into master will increase coverage by 0.06%

@@             master      #2299   diff @@
==========================================
  Files           304        307     +3   
  Lines         21539      21684   +145   
  Methods           0          0          
  Messages          0          0          
  Branches       1852       1865    +13   
==========================================
+ Hits          19520      19665   +145   
  Misses         1665       1665          
  Partials        354        354

Powered by Codecov. Last update d991a66...481bf43

…SaliencyDetection

jni

Hey @parulsethi!

I've left a few comments about general style and syntax.

For tests, have a look at the skimage/feature/tests directory for examples. You should add another file, test_kadir_brady.py, that compares the output of the function with known output for a given input.

For the gallery, see in doc/examples/feature_dection/ for some templates that you can copy. Simply add a file next to those, plot_kadir_brady.py, that illustrates the detector.

Finally, as a general comment, some of the functions are a bit long — you should try to keep the function length shorter than 30-50 lines. If it's longer, see whether you can pull some loops into their own functions.

Hope this helps! =)

jni · 2016-09-23T06:15:40Z

skimage/feature/_kb.py

+from .._shared.utils import assert_nD
+
+
+def kb(image, min_scale=10, max_scale=25, saliency_threshold=0.6, clustering_threshold=7):


I suggest you rename this saliency_kadir_brady. kb is definitely not informative enough!

jni · 2016-09-23T06:19:29Z

skimage/feature/_kb.py

+
+
+def kb(image, min_scale=10, max_scale=25, saliency_threshold=0.6, clustering_threshold=7):
+    """Finds salient regions in the given grayscale image.For each point x


This should be a single sentence followed by a blank line, and should be in present imperative ("Finds salient regions" -> "Find salient regions"). Please review https://www.python.org/dev/peps/pep-0257/ and apply here.

jni · 2016-09-23T06:19:56Z

skimage/feature/_kb.py

+        Features with saliency score satisfying threshold will be considered.
+    clustering_threshold : int, optional
+        Variance among the regions should be smaller than this threshold
+        for sufficient clustering.


Good work on the numpy style parameter list! =)

jni · 2016-09-23T06:20:23Z

skimage/feature/_kb.py

+
+    Returns
+    -------
+    A : (n, 3) ndarray


by convention we use "array of float, shape (n, 3)"

@jni I'm sorry, where do we use this notation? I bet that out : (K, 2) ndarray of float is the most oftenly used version all over the package.

@soupault you are absolutely right! Oops. Dunno where I got that other idea... =\

@parulsethi sorry about this mistake! But, at least, @soupault agree that the N should be capitalised. =)

jni · 2016-09-23T06:23:38Z

skimage/feature/_kb.py

+    assert_nD(image, 2)
+
+    # scales for keypoints
+    scales = np.array([_ for _ in range(min_scale, max_scale, 2)])


Three overlapping critiques here:

Only use _ for values that are discarded. In this case, the values are collected in the array, so you would use e.g. np.array([scale for scale in range(min_scale, max_scale, 2)])

But actually, you're just converting to a list, so instead, prefer np.array(list(range(min_scale, max_scale, 2)))

But, actually, you're just converting to an array, so instead use np.arange(min_scale, max_scale, 2).

=)

jni · 2016-09-23T06:42:27Z

skimage/feature/_kb.py

+            if s_count >= 1:
+                # first derivative in entropy space to calculate weights
+                dif = abs(h - previous_h[:, i])
+                factor = scales[s_count]**2/(2*scales[s_count]-1)


Read PEP8 for suggestions about spacing around mathematical operators

jni · 2016-09-23T06:42:38Z

skimage/feature/_kb.py

+
+
+def prune(candidate_regions, saliency_threshold, v_th, K=7):
+    """ It selects highly salient points that have local support


Same comment about docstrings

jni · 2016-09-23T06:43:11Z

skimage/feature/_kb.py

+    r, c = np.nonzero(mask)
+    nPix = len(r)
+
+    nScales = len(scales)


Don't use camelCase, use under_scores

jni · 2016-09-23T06:43:44Z

skimage/feature/_kb.py

+            # compute the histogram of intensity values in this region
+            patch = image[min_r:max_r, min_c:max_c]
+            h = np.histogram(patch, bins=intensity_edges)[0]
+            h = np.array([_/sum(h) for _ in h])


h /= np.sum(h)

jni · 2016-09-23T06:44:19Z

skimage/feature/_kb.py

+    return np.array([gamma, scale, row, column])
+
+
+def prune(candidate_regions, saliency_threshold, v_th, K=7):


Don't use uppercase arguments.

or value names

parulsethi · 2016-10-02T19:37:21Z

Hey @jni,

Made all the changes requested above, the code is bit changed for using skimage.util.view_as_windows for pixel iteration and also spread out the functions a bit.

Please suggest if any further improvements are required.

jni

@parulsethi this is getting much closer! But I think it will benefit from another round of edits.

I also think that, with a bit of thought and care, it could be converted to support nD images, instead of just 2D.

For now, I would really appreciate it if you could address the comments I've left!

jni · 2016-10-21T03:01:10Z

skimage/feature/_kadir_brady.py

+from ..util import img_as_ubyte, view_as_windows
+from .._shared.utils import assert_nD
+
+def saliency_kadir_brady(image, min_scale=5, max_scale=13, saliency_threshold=0.6, clustering_threshold=2):


@parulsethi PEP8 requires all lines to be wrapped at 80 characters wide.

What editor are you using? Most modern editors will automatically wrap lines for you. I recommend PyCharm.

jni · 2016-10-21T03:13:50Z

skimage/feature/_kadir_brady.py

+        for row in patches:
+            for patch in row:
+                h = np.histogram(patch, bins=intensity_edges)[0]
+                h = h / np.sum(h)


Here you can just use np.histogram(..., normed=True)

jni · 2016-10-21T03:34:16Z

skimage/feature/_kadir_brady.py

+    for s_idx, scale in enumerate(scales):
+        # shape for windows according to current scale
+        radius = scale + 1
+        window_shape = (radius, radius)


This is better described as "sidelen" or "diameter" than "radius"

jni · 2016-10-21T03:35:55Z

skimage/feature/_kadir_brady.py

+    nr, nc = image.shape
+    # find pixels that we are going to examine
+    mask = np.ones((nr - max(scales), nc - max(scales)))
+    r, c = np.nonzero(mask) + max(scales) / 2 + 1


You are trying to add a tuple (np.nonzero(.)) to a float (max(scales) / 2), which is an error.

r and c are assigned here the 2 arrays inside the tuple, so it's actually array+float

In [96]: np.nonzero(mask) Out[96]: (array([0, 0, 1, 1]), array([0, 1, 0, 1])) In [97]: r, c = np.nonzero(mask) In [98]: r Out[98]: array([0, 0, 1, 1]) In [99]: c Out[99]: array([0, 1, 0, 1])

@parulsethi Python evaluates the RHS entirely before assignment, so it first will try to add a tuple to a float, and then unpack the arrays within the tuple:

In [1]: import numpy as np In [2]: mask = np.random.rand(5, 5) > 0.5 In [3]: r, c = np.nonzero(mask) + 1.5 --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-3-036d7e86f56f> in <module>() ----> 1 r, c = np.nonzero(mask) + 1.5 TypeError: can only concatenate tuple (not "float") to tuple

jni · 2016-10-21T03:36:38Z

skimage/feature/_kadir_brady.py

+    # iterate through scales
+    for s_idx, scale in enumerate(scales):
+        # shape for windows according to current scale
+        radius = scale + 1


Why the +1?

to make no. of iterating windows equal to n_pix.

or to just have those windows whose center pixel equals the pixels which are considered from n_pix (or mask)

Sorry, can you elaborate a bit more here? I still don't understand why we don't use the scale as the radius — it seems to be some form of convention, but it doesn't seem justified to me.

In [68]: scales = [5,3] ...: image= np.random.rand(10, 10) ...: nr, nc = image.shape ...: ...: mask = np.ones((nr - max(scales), nc - max(scales))) ...: r, c = np.nonzero(mask) ...: n_pix = len(r) ...: In [69]: n_pix Out[69]: 25

let current_scale = scales[0]
and if i set diameter(thanks for pointing radius being misleading) without doing +1

In [73]: patches = util.view_as_windows(image, current_scale) In [74]: patches.shape Out[74]: (6, 6, 5, 5)

which basically gives no. of patches => 6*6 = 36
but then with +1

In [75]: patches = util.view_as_windows(image, current_scale+1) In [76]: patches.shape Out[76]: (5, 5, 6, 6)

now, no. of patches => 5*5 = 25 which is equal to n_pix computed above.

This is basically because the no. of windows are image.shape - scale+1, so i can either do diameter = scale+1 or do mask = np.ones((nr-max(scales)+1, nc-max(scales)+1)) above which would then give n_pix=36 if we take diameter same as scale.

I guess it's n_pix which should be calculated with (nr-max(scales)+1, nc-max(scales)+1), and diameter should be left same as scale.

@parulsethi yes, this last statement makes the most sense to me. =)

jni · 2016-10-21T03:57:45Z

skimage/feature/_kadir_brady.py

+                h = np.histogram(patch, bins=intensity_edges)[0]
+                h = h / np.sum(h)
+
+                # index of histogram values greater than zero


In comments, don't simply state what the code is doing (that's what the code is for), but why you're doing it. In this case, "find histogram values greater than zero to prevent NaNs when computing 0log(0) during entropy computation"

jni · 2016-10-21T03:58:05Z

skimage/feature/_kadir_brady.py

+    t_r : (N,) ndarray of float
+        Row index of selected regions.
+    t_c : (N,) ndarray of float
+        Cloumn index of selected regions.


jni · 2016-10-21T03:58:33Z

skimage/feature/_kadir_brady.py

+    Parameters
+    ----------
+    candidate_regions : (4, N) ndarray of float
+        A 2d array with each column representing gamma,scale,y,x.


Use "row, column" or "r, c" instead of "y, x".

jni · 2016-10-21T04:01:13Z

skimage/feature/_kadir_brady.py

+            column = np.append(column, center[0])
+            row = np.append(row, center[1])
+            scale = np.append(scale, center[2])
+            gamma = np.append(gamma, t_gamma[index])


This is extremely inefficient... Use lists to append, and convert to array at the end!

thanks for this!

jni · 2016-10-21T04:05:16Z

skimage/feature/_kadir_brady.py

+    cluster = np.zeros((3, k + 1))
+
+    # pruning process
+    for index in range(n):


Can you explain what you're doing here? It seems to me that you should be able to simplify the logic quite a bit. For example, the code block for the first region (under the else) is the same as the code block for a sufficiently-far region. I'm sure you'd be able to condense both code paths into a single clause.

sciunto · 2017-04-25T20:19:24Z

@parulsethi What's the status of this PR for you please? Do you still want to go through @jni's comments?

parulsethi · 2017-04-27T07:29:37Z

Sorry for the delay, I made most of the changes in the review above, will push them here, but was stuck at one of them - replied in the corresponding review comment.

sciunto · 2017-04-27T19:42:22Z

Thanks @parulsethi! Feel free to push the modifications you made so far.

Could you also tick the box (if not done yet) on the right of this page. This is to allow to push on your branch as well. That could be useful for us.

thank you.

parulsethi added 9 commits September 6, 2016 14:04

added KB saliency algo

699a52d

resolved outputs

684388d

flake8 check

365d9ad

added KB saliency algo

c90a528

resolved outputs

fd2d32f

flake8 check

83e8785

updated prune description

796e8d0

updated prune description

f4986df

update descriptions

35ddf1b

parulsethi changed the title ~~Kb saliency detection~~ Implemented Kadir and Brady's Saliency detection Sep 9, 2016

travis fixes

858a2ae

soupault added new feature 💪 Work in progress labels Sep 10, 2016

parulsethi added 2 commits September 15, 2016 04:25

import div for py2

35e9657

Merge branch 'master' of github.com:scikit-image/scikit-image into kb…

7a34b22

…SaliencyDetection

jni requested changes Sep 23, 2016

View reviewed changes

parulsethi added 8 commits September 23, 2016 23:06

added doc example

ec9f3b2

fixed syntax and pixel iteration

a1d0be8

changed function name

f1b31ca

added test case

c36f5f6

changed doc example array

58c91d6

fix doc array

a19701d

fixed doc syntax error

7f5fb49

fixed doc syntax error

481bf43

jni requested changes Oct 21, 2016

View reviewed changes

sciunto added the 🧐 Needs review label Apr 27, 2017

sciunto removed the 💪 Work in progress label Apr 27, 2017

Base automatically changed from master to main February 18, 2021 18:22

mkcor mentioned this pull request Feb 22, 2021

2021's calendar of community management #5169

Closed

grlee77 added 🙏 Feature request and removed type: new feature labels Feb 22, 2022

mkcor mentioned this pull request Feb 28, 2022

2022's calendar of community management #6165

Closed

		from .._shared.utils import assert_nD


		def kb(image, min_scale=10, max_scale=25, saliency_threshold=0.6, clustering_threshold=7):



		def kb(image, min_scale=10, max_scale=25, saliency_threshold=0.6, clustering_threshold=7):
		"""Finds salient regions in the given grayscale image.For each point x



		def prune(candidate_regions, saliency_threshold, v_th, K=7):
		""" It selects highly salient points that have local support

		return np.array([gamma, scale, row, column])


		def prune(candidate_regions, saliency_threshold, v_th, K=7):

Implemented Kadir and Brady's Saliency detection #2299

Are you sure you want to change the base?

Implemented Kadir and Brady's Saliency detection #2299

Conversation

parulsethi commented Sep 9, 2016 • edited Loading

Description

Checklist

References

codecov-io commented Sep 9, 2016 • edited Loading

Current coverage is 90.68% (diff: 100%)

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parulsethi commented Oct 2, 2016

jni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parulsethi Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sciunto commented Apr 25, 2017

parulsethi commented Apr 27, 2017

sciunto commented Apr 27, 2017

parulsethi commented Sep 9, 2016 •

edited

Loading

codecov-io commented Sep 9, 2016 •

edited

Loading

parulsethi Oct 31, 2016 •

edited

Loading