extend draw.ellipse by orientation by Borda · Pull Request #2366 · scikit-image/scikit-image

Borda · 2016-11-03T15:56:29Z

Description

extension of an ellipse to have also parameter orientation as well as ellipse_perimeter
reflecting a "issue" #2135

Checklist

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

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]

Borda · 2016-11-04T18:05:36Z

@sciunto how can I find some more details why the checks failed?

alexdesiqueira · 2016-11-04T20:54:00Z

Hi @Borda, I treated that issue on #2352. Where do these PRs differ?
Thanks!

Borda · 2016-11-05T00:52:28Z

@alexandrejaguar in you case putting orientation as fifth parameter, may break compatibility for some previous calling and you use orientation in degrees wile other functions like draw.ellipse_perimeter already use radians.

JDWarner

I approve the idea and the implementation looks good. Before approval, there are some stylistic changes needed noted below.

You've added a doctest for this functionality, but I would also like a unit test or two added to the test suite. Our in-depth CI testing runs all the doctests, but a typical person testing the package does not (with make test or similar). Unit tests are quite simple; look in the tests subdirectory and follow the form there.

Testing a few orientations and then verifying exactly the same results with rotation plus/minus some multiple of 2*pi would be a good starting point.

JDWarner · 2016-11-05T01:04:24Z

skimage/draw/draw.py

        coordinates. This is useful for ellipses which exceed the image size.
        By default the full extent of the ellipse are used.
+    orientation : float, optional (default 0.)
+        Set the ellipse orientation (rotation)


Specify that this is in radians, relative to the column (c) axis.

JDWarner · 2016-11-05T01:05:03Z

skimage/draw/draw.py

+    The ellipse equation:
+    ``((x * cos(alpha) + y * sin(alpha)) / xradius) ** 2
+                + ((x * sin(alpha) + y * cos(alpha)) / yradius) ** 2 = 1``
+


Please place this info in a new Notes section at the bottom of the docstring.

JDWarner · 2016-11-05T01:07:15Z

skimage/draw/draw.py



-def ellipse(r, c, yradius, xradius, shape=None):
+def ellipse(r, c, yradius, xradius, shape=None, orientation=0.):


As these are not kwargs, renaming yradius -> rradius and xradius -> cradius should be done for consistency. It won't break anything. Also update the relevant part of the docstring.

Also, from an API standpoint I think I prefer rotation to orientation.

yradius and xradius are already renamed in master branch.
@Borda please, do a rebase.

@soupault I made pull in master (but it says it is up-to-date) and rebase master nothing changed.

@Borda please, double check :). I'm pretty sure the latest signature is different - https://github.com/scikit-image/scikit-image/blob/master/skimage/draw/draw.py#L20.

JDWarner · 2016-11-05T01:07:57Z

skimage/draw/draw.py

-    """Generate coordinates of points within ellipse bounded by shape."""
+def _ellipse_in_shape(shape, center, radiuses, orientation=0.):
+    """ Generate coordinates of points within ellipse bounded by shape.
+    see: http://www.maa.org/external_archive/joma/Volume8/Kalman/General.html


Put the reference in a References section at the bottom of the docstring - look elsewhere in the package for the proper Sphinx formatting for this.

JDWarner · 2016-11-05T01:13:17Z

skimage/draw/draw.py

+    :param center: (float, float) position of center
+    :param radiuses: (float, float) size of the two half axis
+    :param orientation: float, orientation in radians
+    :return: [int], [int] coordinates of ellipse


Please use NumPy documentation standards, e.g.,

""" Parameters ---------- shape : iterable of ints Shape of the input image. Must be length 2. center : iterable of floats (row, column) position of center (in what coordinates? Presumably index coordinates?). radii : iterable of floats Size of two half axes (for row and column, prior to orientation presumably?) orientation : float, optional Rotation of the ellipse defined by the above, in radians, with respect to the column-axis. Returns ------- rows : iterable of ints Row coordinates representing values within the ellipse. cols : iterable of ints Corresponding column coordinates representing values within the ellipse. """

or similar.

JDWarner · 2016-11-05T01:15:38Z

skimage/draw/draw.py

    bounding_shape = lower_right - upper_left + 1

-    rr, cc = _ellipse_in_shape(bounding_shape, shifted_center, radiuses)
+    rr, cc = _ellipse_in_shape(bounding_shape, shifted_center, radiuses,


Please use the proper plural radii throughout.

alexdesiqueira · 2016-11-05T07:26:28Z

@Borda using theta as degrees should be easy to solve. Also, shape is commonly the last argument according to other functions (respecting the API). @soupault @sciunto @JDWarner I submitted #2352 before this and no one checked that PR. Is there anything wrong?

Conflicts: skimage/draw/draw.py * update according PR comments * add unity test for multiple rotations

* add unity test for multiple rotations

Borda · 2016-11-08T07:24:06Z

May I ask what is going on with the "ERROR: Unexpected indentation." @soupault @JDWarner
the compilation on appveyor is fine but it crashes on travis-ci for the doc string...

sciunto · 2016-11-09T07:51:29Z

Appveyor doesn't build the doc, so it passes. Check your example, the problem is in there.

Borda · 2016-11-09T10:17:40Z

I see, the fail comes at line 38, which was '.. rubric:: Notes', but it is the same as further '.. rubric:: Examples' so is there a list of possible rubrics?

sciunto · 2016-11-09T10:50:03Z

skimage/draw/draw.py

           [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
           [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]], dtype=uint8)

+    >>> img = np.zeros((10, 12), dtype=np.uint8)


I would remove the previous example in favor of your example, which is more general. One is enough I think and it has the advantage to keep the docstring as short as possible.

sciunto · 2016-11-09T10:51:53Z

Just by eye inspection, I do not detect the problem... I need to test your commit. However, I left a little comment. To me, the section's names are fine.

soupault · 2016-11-09T12:58:45Z

skimage/draw/draw.py

+    Notes
+    -----
+    The ellipse equation:
+    ``((x * cos(alpha) + y * sin(alpha)) / x_radius) ** 2


I think that the issue is probably here.
Please, check with the different formatting (https://github.com/scikit-image/scikit-image/blob/master/skimage/transform/_geometric.py#L302).

…t-image into add_ellipse_rotation Conflicts: skimage/draw/draw.py

Borda · 2016-11-11T13:27:36Z

@sciunto @soupault Do you have any idea what is wrong with the doc string?
@JDWarner Do you agree with changes according review?

soupault · 2016-11-14T13:48:47Z

skimage/draw/draw.py

+    -----
+    The ellipse equation::
+
+    ((x * cos(alpha) + y * sin(alpha)) / x_radius) ** 2


I think Sphinx expects an indentation here. Try this:

The ellipse equation:: ((x * cos(alpha) + y * sin(alpha)) / x_radius) ** 2 + ((x * sin(alpha) - y * cos(alpha)) / y_radius) ** 2 = 1

codecov-io · 2016-11-14T14:41:39Z

Current coverage is 90.64% (diff: 100%)

Merging #2366 into master will decrease coverage by <.01%

@@             master      #2366   diff @@
==========================================
  Files           305        305          
  Lines         21587      21625    +38   
  Methods           0          0          
  Messages          0          0          
  Branches       1854       1858     +4   
==========================================
+ Hits          19568      19602    +34   
- Misses         1665       1668     +3   
- Partials        354        355     +1

Powered by Codecov. Last update 86edbf0...d77abb9

Borda · 2016-12-07T18:07:14Z

@JDWarner Are you fine with changes according your review? :)

JDWarner · 2016-12-07T18:17:15Z

Looks good now - thanks for your contribution, @Borda, and sorry for the delay!

init task

1fc7972

Borda changed the title ~~init task~~ extend draw.ellipse by orientation Nov 3, 2016

sciunto added ⏩ type: Enhancement Improve existing features 💪 Work in progress labels Nov 3, 2016

extending the ellipse by orientation

a2da948

JDWarner requested changes Nov 5, 2016

View reviewed changes

Borda added 6 commits November 6, 2016 09:08

add compute rotated bounding box

3ffdf76

Merge branch 'master' into add_ellipse_rotation

78d6f93

Conflicts: skimage/draw/draw.py * update according PR comments * add unity test for multiple rotations

* update according PR comments

4947d87

* add unity test for multiple rotations

bug fix in equation

198001b

add test for estimate ellipse orientation

a87e1d0

add test for estimate ellipse orientation

9da9b08

moved Notes to bottom of docString

b027976

sciunto reviewed Nov 9, 2016

View reviewed changes

soupault reviewed Nov 9, 2016

View reviewed changes

Borda added 2 commits November 9, 2016 15:47

docString change equation

fcf2dce

Merge branch 'add_ellipse_rotation' of https://github.com/Borda/sciki…

6f23cee

…t-image into add_ellipse_rotation Conflicts: skimage/draw/draw.py

soupault reviewed Nov 14, 2016

View reviewed changes

change equation in Notes

d77abb9

JDWarner approved these changes Dec 7, 2016

View reviewed changes

JDWarner merged commit 2659b3b into scikit-image:master Dec 7, 2016

soupault removed the 💪 Work in progress label Dec 7, 2016

This was referenced Dec 7, 2016

Adding orientation to draw.ellipse. #2352

Closed

Add rotation to draw.ellipse #2135

Closed

Borda deleted the add_ellipse_rotation branch December 7, 2016 18:25

JDWarner mentioned this pull request Dec 7, 2016

latex_use_modeindex deprecation warning always thrown sphinx-doc/sphinx#3204

Closed



		def ellipse(r, c, yradius, xradius, shape=None):
		def ellipse(r, c, yradius, xradius, shape=None, orientation=0.):

Uh oh!

Conversation

Borda commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

References

Uh oh!

Borda commented Nov 4, 2016

Uh oh!

alexdesiqueira commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Borda commented Nov 5, 2016

Uh oh!

JDWarner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexdesiqueira commented Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Borda commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sciunto commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Borda commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sciunto commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Borda commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 90.64% (diff: 100%)

Uh oh!

Borda commented Dec 7, 2016

Uh oh!

JDWarner commented Dec 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Borda commented Nov 3, 2016 •

edited

Loading

alexdesiqueira commented Nov 4, 2016 •

edited

Loading

alexdesiqueira commented Nov 5, 2016 •

edited

Loading

Borda commented Nov 8, 2016 •

edited

Loading

sciunto commented Nov 9, 2016 •

edited

Loading

codecov-io commented Nov 14, 2016 •

edited

Loading