skimage.segmentation.quickshift signature is missing from API docs #2017

soupault · 2016-03-19T09:30:11Z

Should fix #2005 .

soupault · 2016-03-19T09:50:03Z

That's interesting:

[1/1] Cythonizing /home/travis/build/scikit-image/scikit-image/skimage/segmentation/_quickshift.pyx
Error compiling Cython file:
------------------------------------------------------------
...
                        dist = 0
                        for channel in range(channels):
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
                                                    ^
------------------------------------------------------------
skimage/segmentation/_quickshift.pyx:115:53: Coercion from Python not allowed without the GIL
Error compiling Cython file:
------------------------------------------------------------
...
                        dist = 0
                        for channel in range(channels):
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
                                                    ^
------------------------------------------------------------
skimage/segmentation/_quickshift.pyx:115:53: Operation not allowed without gil
Error compiling Cython file:
------------------------------------------------------------
...
                        dist = 0
                        for channel in range(channels):
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
                                              ^
------------------------------------------------------------
skimage/segmentation/_quickshift.pyx:115:47: Converting to Python object not allowed without gil
Error compiling Cython file:
------------------------------------------------------------
...
                        dist = 0
                        for channel in range(channels):
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
                                                         ^
------------------------------------------------------------
skimage/segmentation/_quickshift.pyx:115:58: Operation not allowed without gil
Error compiling Cython file:
------------------------------------------------------------
...
                        dist = 0
                        for channel in range(channels):
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
                                                                      ^
------------------------------------------------------------
skimage/segmentation/_quickshift.pyx:115:71: Operation not allowed without gil
Traceback (most recent call last):
  File "setup.py", line 146, in <module>
    **extra
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/core.py", line 135, in setup
    config = configuration()
  File "setup.py", line 69, in configuration
    config.add_subpackage('skimage')
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 1002, in add_subpackage
    caller_level = 2)
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 971, in get_subpackage
    caller_level = caller_level + 1)
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 908, in _get_configuration_from_setup_py
    config = setup_module.configuration(*args)
  File "skimage/setup.py", line 24, in configuration
    config.add_subpackage('segmentation')
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 1002, in add_subpackage
    caller_level = 2)
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 971, in get_subpackage
    caller_level = caller_level + 1)
  File "/home/travis/venv/lib/python2.6/site-packages/numpy/distutils/misc_util.py", line 908, in _get_configuration_from_setup_py
    config = setup_module.configuration(*args)
  File "skimage/segmentation/setup.py", line 17, in configuration
    cython(['_quickshift.pyx'], working_path=base_path)
  File "/home/travis/build/scikit-image/scikit-image/skimage/_build.py", line 50, in cython
    cythonize(pyxfile)
  File "/home/travis/venv/lib/python2.6/site-packages/Cython/Build/Dependencies.py", line 877, in cythonize
    cythonize_one(*args)
  File "/home/travis/venv/lib/python2.6/site-packages/Cython/Build/Dependencies.py", line 997, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: /home/travis/build/scikit-image/scikit-image/skimage/segmentation/_quickshift.pyx

blink1073 · 2016-03-19T09:51:24Z

skimage/segmentation/_quickshift.pyx

@@ -14,7 +14,7 @@ from ..util import img_as_float
 from ..color import rgb2lab


-def quickshift(image, ratio=1., float kernel_size=5, max_dist=10,
+def quickshift(image, ratio=1.0, kernel_size=5, max_dist=10,


kernel_size is used in a nested for-loop in this function. It seems like we should precompute the value used in that loop and store it.

(as a cdef)

Yes, already working on that.

blink1073 · 2016-03-19T09:52:36Z

@soupault, that error makes sense given my comments above.

blink1073 · 2016-03-19T11:19:26Z

skimage/segmentation/_quickshift.pyx

@@ -112,7 +113,7 @@ def quickshift(image, ratio=1.0, kernel_size=5, max_dist=10,
                            dist += (current_pixel_p[channel] -
                                     image_c[r_, c_, channel])**2
                        dist += (r - r_)**2 + (c - c_)**2
-                        densities[r, c] += exp(-dist / (2 * kernel_size**2))
+                        densities[r, c] += exp(-dist / (2 * kernel_size_c**2))


How about cdef float kernel_size_sq = kernel_size**2 instead (it was a float before, and this saves taking the square each time).

Yes, sorry, you are right.

I am fairly certain that the compiler is smart to optimize, but why not go all the way and make it inv_neg_kernel_size_sqr = -0.5 / kernel_size**2? The loss in precision shouldn't matter too much in this case I assume.

@ahojnnes I fully agree, but, in my opinion, this should be an another PR. One can find other things (incl. TODO ⚡) which should be addressed.

blink1073 · 2016-03-19T11:46:11Z

LGTM

ahojnnes · 2016-03-19T11:55:05Z

skimage/segmentation/_quickshift.pyx

-    cdef int kernel_size_c = kernel_size
-    cdef int w = 3 * kernel_size_c
+    cdef float kernel_size_sq = kernel_size**2
+    cdef int w = int(3 * kernel_size)


hm, maybe ceil?

@ahojnnes Should I use cnp.ceil or np.ceil here?

Won't matter too much, but cnp.ceil should be a wrapper around the ceil function in <math.h> and therefore faster in Cython code.

Ok, clear. I've already made the change.

ahojnnes · 2016-03-19T14:13:14Z

THanks @soupault!

skimage.segmentation.quickshift signature is missing from API docs

soupault added bug 📄 type: Documentation Updates, fixes and additions to documentation labels Mar 19, 2016

blink1073 reviewed Mar 19, 2016
View reviewed changes

ahojnnes reviewed Mar 19, 2016
View reviewed changes

Fixed quickshift signature, added cdef on kernel_size

85b7f6b

soupault force-pushed the fix_doc_quickshift branch from 7b8a677 to 85b7f6b Compare March 19, 2016 12:27

ahojnnes added a commit that referenced this pull request Mar 19, 2016

Merge pull request #2017 from soupault/fix_doc_quickshift

1e9aac2

skimage.segmentation.quickshift signature is missing from API docs

ahojnnes merged commit 1e9aac2 into scikit-image:master Mar 19, 2016

soupault deleted the fix_doc_quickshift branch March 19, 2016 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skimage.segmentation.quickshift signature is missing from API docs #2017

skimage.segmentation.quickshift signature is missing from API docs #2017

soupault commented Mar 19, 2016

soupault commented Mar 19, 2016

blink1073 Mar 19, 2016

blink1073 Mar 19, 2016

soupault Mar 19, 2016

blink1073 commented Mar 19, 2016

blink1073 Mar 19, 2016

soupault Mar 19, 2016

ahojnnes Mar 19, 2016

soupault Mar 19, 2016

blink1073 commented Mar 19, 2016

ahojnnes Mar 19, 2016

soupault Mar 19, 2016

soupault Mar 19, 2016

ahojnnes Mar 19, 2016

soupault Mar 19, 2016

ahojnnes commented Mar 19, 2016

skimage.segmentation.quickshift signature is missing from API docs #2017

skimage.segmentation.quickshift signature is missing from API docs #2017

Conversation

soupault commented Mar 19, 2016

soupault commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Mar 19, 2016

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

blink1073 commented Mar 19, 2016

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

ahojnnes commented Mar 19, 2016