Mdmueller cython wrappers: Made Python wrappers for public Cython functions #2303

sciunto · 2016-09-17T12:39:35Z

Replaces #1432

Closes #1323, #2047

sciunto · 2016-09-17T12:54:28Z

@scikit-image/core please, feel free to help to sort out the last issues. We need to merge this PR quickly.

ahojnnes

There are a few comments, otherwise this looks good. But there seems to be a merge conflict.

ahojnnes · 2016-09-17T14:11:20Z

skimage/draw/draw.py

+    Bresenham method is also known as midpoint circle algorithm.
+    Anti-aliased circle generator is available with `circle_perimeter_aa`.
+
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:11:36Z

skimage/draw/draw.py

+
+    Notes
+    -----
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:11:46Z

skimage/draw/draw.py

+    Wu's method draws anti-aliased circle. This implementation doesn't use
+    lookup table optimization.
+
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:11:52Z

skimage/draw/draw.py

+
+    Notes
+    -----
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:11:58Z

skimage/draw/draw.py

+    The algorithm is the rational quadratic algorithm presented in
+    reference [1]_.
+
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:12:22Z

skimage/feature/corner.py

+
+    Notes
+    -----
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:12:30Z

skimage/segmentation/quickshift_wrapper.py

+    segmentation, though this is not strictly necessary. For this to work, the
+    image must be given in RGB format.
+
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:12:57Z

skimage/transform/hough_transform.py

-    >>> result.tolist()
-    [(10, 10.0, 10.0, 8.0, 6.0, 0.0)]
+    >>> result
+    [(10, 10.0, 8.0, 6.0, 0.0, 10.0)]


Why did this change?

ahojnnes · 2016-09-17T14:13:02Z

skimage/transform/hough_transform.py


    Notes
    -----
    The accuracy must be chosen to produce a peak in the accumulator
    distribution. In other words, a flat accumulator distribution with low
    values may be caused by a too low bin size.

+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

ahojnnes · 2016-09-17T14:13:13Z

skimage/transform/hough_transform.py

+
+    Notes
+    -----
+    This function is a wrapper for Cython code.


I don't think this comment is necessary...

sciunto · 2016-09-17T14:24:14Z

I think some tests do not pass...

sciunto · 2016-09-17T15:48:11Z

I fixed few additional regressions (thanks unittests).

sciunto · 2016-09-17T17:08:41Z

OK. I think I'm done fixing the tests.

codecov-io · 2016-09-18T05:51:48Z

Current coverage is 90.63% (diff: 100%)

Merging #2303 into master will increase coverage by 0.01%

@@             master      #2303   diff @@
==========================================
  Files           304        305     +1   
  Lines         21540      21563    +23   
  Methods           0          0          
  Messages          0          0          
  Branches       1851       1852     +1   
==========================================
+ Hits          19521      19544    +23   
  Misses         1665       1665          
  Partials        354        354

Powered by Codecov. Last update 9a516ed...ab5dc99

soupault · 2016-09-18T06:46:35Z

skimage/draw/draw.py

@@ -272,3 +268,354 @@ def set_color(img, coords, color, alpha=1):
    vals = img[rr, cc] * (1 - alpha)

    img[rr, cc] = vals + color
+
+
+def line(y1, x1, y2, x2):


I'd suggest to use 0-based indexing: y0, y1, ....

soupault · 2016-09-18T06:46:44Z

skimage/draw/draw.py

+    return _line(y1, x1, y2, x2)
+
+
+def line_aa(y1, x1, y2, x2):


soupault · 2016-09-18T06:53:32Z

skimage/draw/draw.py

@@ -156,7 +153,6 @@ def polygon_perimeter(cr, cc, shape=None, clip=False):

    Examples
    --------
-    >>> from skimage.draw import polygon_perimeter


Why remove this line?

soupault · 2016-09-18T06:55:20Z

skimage/draw/draw.py

+
+
+def circle_perimeter(cy, cx, radius,
+                     method='bresenham', shape=None):


Can this be put in a single line?

soupault · 2016-09-18T06:58:20Z

skimage/draw/draw.py

+
+    Returns
+    -------
+    rr, cc : (N,) ndarray of int


As I can understand, this is not correct. _bezier_curve returns arrays in a reverse order (px, py ==> cc, rr).

If that's correct, it means that the API is wrong (see example below...). I vote to fix that elsewhere.

soupault · 2016-09-18T07:00:01Z

skimage/measure/pnpoly.py

+        Shape of the grid.
+    verts : (V, 2) array
+        Specify the V vertices of the polygon, sorted either clockwise
+        or anti-clockwise.  The first point may (but does not need to be)


Fix double whitespace.

soupault · 2016-09-18T07:04:19Z

skimage/transform/hough_transform.py

+    hspace_max = ndimage.maximum_filter1d(hspace, size=distance_size, axis=0,
+                                          mode='constant', cval=0)
+    hspace_max = ndimage.maximum_filter1d(hspace_max, size=angle_size, axis=1,
+                                          mode='constant', cval=0)


I wonder why not just to use nd version of maximum_filter? (Not a blocker for this PR.)

soupault · 2016-09-18T07:06:37Z

skimage/transform/hough_transform.py

@@ -257,43 +153,30 @@ def hough_circle(image, radius, normalize=True, full_output=False):
        Hough transform accumulator for each radius.
        R designates the larger radius if full_output is True.
        Otherwise, R = 0.
-
-    Examples


I agree to remove this for now - is a very complicated example. Any plans to add something instead in the near future?

I even didn't figure out that was removed... hum.

soupault · 2016-09-18T07:09:12Z

Thanks for pushing this, @sciunto! Few remarks from my side, then is good to go.

sciunto · 2016-09-18T08:35:47Z

I made some changes. @soupault I do not disagree with the rest, but, if you don't mind, I would prefer to see new issues and this one merged. It's already thick. :)

soupault · 2016-09-18T09:28:01Z

@sciunto Sure.
You forgot to rename variables in returns. :)

sciunto · 2016-09-23T06:51:45Z

@soupault I opened #2319, so it's not blocking this PR and @amccaugh can work on his feature.

Thanks!

soupault · 2016-09-26T16:20:11Z

@sciunto this is blocking now -> be94c6f#diff-17d59a5c1e414486337775a134925622L313 :). 0-indexing there as well.

sciunto · 2016-09-26T18:07:10Z

Thanks! I didn't see it.

soupault · 2016-09-26T18:13:36Z

@sciunto would you like to squash the commits, while you are here?

sciunto · 2016-09-26T18:24:22Z

I regrouped half of them. I kept some of them for the history.

soupault · 2016-09-26T19:32:36Z

This LGTM. @ahojnnes would you like to give it a quick look?

ahojnnes · 2016-09-26T19:42:58Z

LGTM.

soupault · 2016-09-26T19:58:24Z

Thanks @mdmueller, @sciunto! Great work, gentlemen!

mdmueller and others added 5 commits September 17, 2016 14:35

Made Python wrappers for public Cython functions

a42f9a6

Fixed Travis CI errors

d5a5244

Changes to docstring style

4b5b027

Suggested style changes, e.g. adding newlines between functions

0c753ef

PEP8

59adb42

sciunto added the 🔧 type: Maintenance Refactoring and maintenance of internals label Sep 17, 2016

sciunto added this to the 0.13 milestone Sep 17, 2016

sciunto assigned ahojnnes and sciunto Sep 17, 2016

sciunto changed the title ~~Mdmueller cython wrappers~~ Mdmueller cython wrappers: Made Python wrappers for public Cython functions Sep 17, 2016

ahojnnes requested changes Sep 17, 2016

View reviewed changes

ahojnnes approved these changes Sep 17, 2016

View reviewed changes

sciunto force-pushed the mdmueller-cython-wrappers2 branch from 37c96da to e0d5e8e Compare September 17, 2016 14:51

sciunto force-pushed the mdmueller-cython-wrappers2 branch from a9e0c12 to 3633ecb Compare September 17, 2016 17:08

sciunto added the 🧐 Needs review label Sep 18, 2016

sciunto assigned soupault Sep 18, 2016

soupault requested changes Sep 18, 2016

View reviewed changes

sciunto mentioned this pull request Sep 23, 2016

bezier_curve returns arrays in a reverse order (column/line convention) #2319

Closed

3 tasks

sciunto added 4 commits September 26, 2016 20:20

Fix imports and doctests

f679d70

Define None values in wrappers and Fix regression

83dac0f

Remove wrapper of a wrapper...

2c75544

Minor fixes

ab5dc99

sciunto force-pushed the mdmueller-cython-wrappers2 branch from 5d72078 to ab5dc99 Compare September 26, 2016 18:22

soupault mentioned this pull request Sep 26, 2016

Refactor variable names in skimage.draw #2321

Merged

soupault approved these changes Sep 26, 2016

View reviewed changes

soupault removed the 🧐 Needs review label Sep 26, 2016

soupault merged commit 5d0352f into scikit-image:master Sep 26, 2016

sciunto deleted the mdmueller-cython-wrappers2 branch September 26, 2016 19:56

soupault mentioned this pull request Sep 26, 2016

Functions' signatures are missing from the docs #2047

Closed

11 tasks

soupault mentioned this pull request Sep 26, 2016

Convert draw/_draw.pyx to "r/c" naming conventions #1602

Closed



		def circle_perimeter(cy, cx, radius,
		method='bresenham', shape=None):

Mdmueller cython wrappers: Made Python wrappers for public Cython functions #2303

Mdmueller cython wrappers: Made Python wrappers for public Cython functions #2303

Conversation

sciunto commented Sep 17, 2016 • edited by soupault

sciunto commented Sep 17, 2016

ahojnnes 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

sciunto commented Sep 17, 2016

sciunto commented Sep 17, 2016

sciunto commented Sep 17, 2016

codecov-io commented Sep 18, 2016 • edited

Current coverage is 90.63% (diff: 100%)

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

soupault commented Sep 18, 2016

sciunto commented Sep 18, 2016

soupault commented Sep 18, 2016

sciunto commented Sep 23, 2016

soupault commented Sep 26, 2016

sciunto commented Sep 26, 2016

soupault commented Sep 26, 2016

sciunto commented Sep 26, 2016

soupault commented Sep 26, 2016

ahojnnes commented Sep 26, 2016

soupault commented Sep 26, 2016

sciunto commented Sep 17, 2016 •

edited by soupault

codecov-io commented Sep 18, 2016 •

edited