Remove duplicates from perimeter functions in draw module

[lagru]

Description

• Fixes Duplicate coordinates produced by draw.circle_perimeter #4141. As described, the functions circle_perimeter, circle_perimeter_aa and ellipse_perimeter might return duplicate coordinates. This is likely unintuitive.
• Furthermore I took the chance to slightly reduce the Cython footprint for the skimage.draw module.

Another point to discuss: as you may see the test skimage.draw.tests.test_draw.Test_circle_perimeter_aa.test_normal fails. The cause for the mismatching pixels is that the function returns duplicates with conflicting intensity values. I'm am not sure how to handle this. Perhaps this hints at a bug inside circle_perimeter_aa?

Checklist

• Docstrings for all functions
• [ ] Gallery example in ./doc/examples (new features only)
• [ ] Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests
• Clean style in the spirit of PEP8

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.
• Consider backporting the PR with @meeseeksdev backport to v0.14.x

[lagru]

I took the liberty to hide the PEP8 related comment as resolved as it doesn't like the formatting of the reference arrays in the tests.

[hmaarrfk]

Is there a better way to get unique coordinates than to check the whole array for uniqueness?

[lagru]

Maybe. I don't fully understand the algorithms that generate the duplicates. It's easy to see why some duplicates are generated (e.g. in _circle_perimeter if one of the variables r or c is 0 the leading signs become meaningless).

Do you feel that performance is a problem here? I did a quick test and using np.unique slows down circle_perimeter by the factor 4 (performance seems similar when commenting that out). I'm surprised that stacking + np.unique + indexing has that big of an impact.

[hmaarrfk]

i guess you could check if r == c i many cases, but in the aa case it is a little trickier.

[lagru]

That's an idea. But we have to check for 0 as well. This

if r == c == 0:
    rr.append(r)
    cc.append(c)
elif r == 0:
    rr.extend([r, r, c, -c])
    cc.extend([c, -c, r, r])
elif c == 0:
    rr.extend([r, -r, c, c])
    cc.extend([c, c, r, -r])
elif r == c:
    rr.extend([r, -r, r, -r])
    cc.extend([c, c, -c, -c])
else:
    rr.extend([r, -r, r, -r, c, -c, c, -c])
    cc.extend([c, c, -c, -c, r, r, -r, -r])

seems to work. Did a few quick tests and this is nearly the same as the old implementation. The np.unique approach is consistently 3 to 4x slower.

[lagru]

That solution would work for circle_perimeter_aa as well but the cases would get kind of tedious because we'd have to consider r - 1 and 1 - r as well...

[jni]

@lagru I don't know anything about anti-aliased circle generation. My git sleuthing suggests that this was originally implemented by @sciunto, who perhaps has ideas about the duplicate but inconsistent values there?

I definitely don't think we need classes for tests, fwiw. Glad that bit's gone... =)

Other than that, I made a grand total of one useful suggestion (I think), which is to implement the coordinate filtering within _inside_image to avoid so much repeated boilerplate code.

[jni]

This pattern is repeated over and over. If you're going to change the _inside_image logic (which seems unnecessary, but ok), perhaps we should let the function do the work of filtering as well? ie:

rr, cc = _inside_image((rr, cc), shape)
(rr, cc), values = _inside_image((rr, cc), shape, values=values)
(pp, rr, cc), values = _inside_image((pp, rr, cc), shape, values=values)

If we want to avoid changing the return based on the input, we can always return values, returning None if none were provided.

[sciunto]

I have not idea ether. That's why I made a +1 on @lagru 's comment in the issue. I remember I read some papers for the general picture, but at that time, I basically followed the implementation linked in the references.

[lagru]

This pattern is repeated over and over.

Yes but considering how stupidly simple that snippet is one could argue that the code is actually better. But that's not something I feel strongly about. 😉

[rfezzani]

Since there is only two options for method, cmethod can be initialized here to b'b' to avoid setting it to b'a' and remove the second test line 278.

[lagru]

I would agree with you if there wasn't also a calculation and assignment to d in both branches. Recalculating d to save an elif-branch seems more confusing to me.

[lagru]

test_circle_perimeter_aa fails for because a9fc1dc exposes the bug documented in #6541.