Added float support for radius and centre in circle_perimeter funct...

[aman-harness]

...ion
Fixes #1415

[coveralls]

Coverage increased (+0.0%) to 92.71% when pulling 9a8f8bd on bewithaman:fbb into e91dcba on scikit-image:master.

[sciunto]

I would add in the docstring that float values are rounded.

[stefanv]

Rounding the values is not the correct solution.

[sciunto]

I don't know if it's not correct, but for sure, not ideal :)

For Bresenham, we must consider the closest pixels from the circle. For Andres, it might be more subtle.
And the same work must be done for other shapes for consistency.

[aman-harness]

@stefanv @sciunto I tried to search about these methods for non integer radius and center but couldn't found much. Can you tell me which approximation method should I apply here?

[coveralls]

Coverage increased (+0.0%) to 92.71% when pulling d0fc21d on bewithaman:fbb into e91dcba on scikit-image:master.

[coveralls]

Coverage increased (+0.0%) to 92.71% when pulling dc52ded on bewithaman:fbb into e91dcba on scikit-image:master.

[vighneshbirodkar]

Seems good to me. @stefanv Can you have a look ?

[stefanv]

float or np.float64

[stefanv]

That looks fine to me.

[sciunto]

looks like a square. A larger radius perhaps?

[vighneshbirodkar]

@bewithaman
Maybe you can choose a large matrix and set it's values by using the equation of a circle x**2 + y**2 = r**2 for testing

[aman-harness]

@vighneshbirodkar Are you suggesting to match just few points or whole matrix, because in the second case I think tests will fail.

[stefanv]

@bewithaman I think @vighneshbirodkar is suggesting that you make the circle large enough that a person can see it is an actual circle (because here it looks like a square)

[aman-harness]

@jaimefrio
Thanks for the correction. All the test passed.

\ cc @stefanv

[stefanv]

Would it make sense to do ry = np.round(ry).astype(int) here to avoid allthe astype calls further down? Same for rx.

[jaimefrio]

Some general comments:

1. Unless there is a specific skimage policy about this, you probably want to use double, not float.
2. Does it make sense to change this one function, without changing all of the other drawing functions? It may be more confusing than helpful to not have a uniform interface.

And a major objection:

I'm afraid the implementation is not correct... I haven't been able to find a copy of the Andres paper that isn't behind a paywall, but the Bresenham algorithm, as implemented, makes lots of implicit assumptions on it working on integers. Translating everything to floating point verbatim and then rounding isn't going to necessarily work, especially given the following behavior of np.round:

>>> np.round([3.5, 4.5])
array([ 4.,  4.])

i.e. ties are resolved to the nearest even number. If you give it an integer radius and at least one of the center coordinates has fractional part 0.5, you are going to get an awful result:

>>> a = np.zeros((10, 10), np.uint8)
>>> a[circle_perimeter(4.5, 4.5, 3)] = 1
>>> a
array([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
       [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
       [0, 0, 1, 0, 1, 0, 1, 0, 0, 0],
       [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
       [0, 0, 1, 0, 0, 0, 0, 0, 1, 0],
       [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
       [0, 0, 1, 0, 0, 0, 1, 0, 1, 0],
       [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
       [0, 0, 0, 0, 1, 0, 1, 0, 0, 0],
       [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]], dtype=uint8)

You could reimplement the algorithm, keeping a floating point error, but still using integer coordinates. But I don't think you can get away with computing 1/8-th of the circle only, or with doing the computations for the circle centered at (0, 0) and then simply offsetting it.

Before going down that route I'd reassess the need for floating point values in this function...

[stefanv]

@jaimefrio I don't think whether you specify the radius as a floating point value or as an integer makes much of a difference--the circle path will always be "along" non-integers. It may be that Bresenheim was always broken for circles, in which case we should rewrite it anyway.

float vs int -- np.float should translate to double anyway, no? I don't think we've set any policy around this so far.

[stefanv]

@jaimefrio Oh, and thank you for spotting this oversight!

[jaimefrio]

Oh yes, np.float is not C's float, my bad, disregard that comment.

The issue with the interface is that, with its current implementation, floating point values passed in will raise an error. After this PR they won't for circle_perimeter, but they still will for all other functions in the draw module. This is what may confuse users, I think.

I don't think Bresenham is broken. The issue with this PR is that it is selecting a sequence of non-integral points that it then rounds. There is the issue with numpy's round pointed above. My other concern is that Bresenham's algorithm is supposed to select consecutive pixels where (x-cx)**2 + (y-cy)**2 <= r**2 while maximizing (x-cx)**2 + (y-cy)**2, and because of the rounding I don't think this PR is doing that anymore, e.g. circle_perimeter(0, 0.6, 3) will select (0, 4), a point for which the inequality does not hold. Now this may not be relevant if all you are after is drawing a circle that looks pretty, but I wouldn't be surprised if a skimage user was expecting more than pretty from this function.

[JDWarner]

For the record, NumPy's rounding behavior is correct per IEEE standards. You have to do it that way to minimize systematic error.

I'm not intimately familiar with the Brensham algorithm though, so I'll defer judgement on the rest of the discussion.

[stefanv]

@jaimefrio Indeed, we need a contiguous circle.

[stefanv]

I don't think the Bresenham algorithm required integer parameters. @jaimefrio Could you take another look, also at this?

http://en.wikipedia.org/wiki/Midpoint_circle_algorithm

[stefanv]

Would a different rounding algorithm (not round to even—floor(x + 0.5)) help here? Either way, this needs further investigation (and an update in the implementation) before it can be considered for inclusion (the example @jaimefrio gives is clearly not correct).