Fix Circle to remove singular edge

[GlennWSo]

Commits

• added test assert for circle: all edges of the circle should have approx the same length
• Remove duplicate point, by setting endpoint=False for theta This will not change the total number of points, but the spacing of theta, now passes the updated test

Overview

Fixing bugg in pv.Circle.
See linked issue below, for a more detailed bugg description:
resolves #3709

Breaking Change:

This change can break functions depending on pyvista.Circle

Example of broken function:

The fallowing function works with current pyvista, but with the suggested changes it will be broken.
With the changes, the hexagon function will incorrectly create a 7-sided polygon.

import pyvista as pv


def hexagon(r: float) -> pv.PolyData:
    """
    creates a regular hexagon(polygon with 6 equal sides) in plane: Z=0,
     with points <r> distance from origo
    """
    # create one extra point and edge, since circle always make one incorrect edge
    hex = pv.Circle(radius=r, resolution=6 + 1)
    return hex.clean(tolerance=0.001)  # clean away the edge with near zero length

With the suggested pyvista changes, the hexagon implementation would need too be updated like so:

import pyvista as pv

def hexagon(r: float) -> pv.PolyData:
    """
    creates a regular hexagon(polygon with 6 equal sides) in plane: Z=0,
     with points <r> distance from origo
    """
    hex = pv.Circle(radius=r, resolution=6)
    return hex

Details

• < feature1 or bug1 description >

import pyvista as pv
import numpy as np

circle4 = pv.Circle(radius=1, resolution=4)
edge_vectors = np.roll(circle4.points, shift=1, axis=0)/4  - circle4.points
print(edge_vectors)
circle4.plot()

now prints:

[[-1., -1., 0.],
[ 1., -1., 0.],
[ 1., 1., 0.],
[-1., 1., 0.]])

old prints:

[[ 0. -0. 0. ]
[ 1.5 -0.866 0. ]
[ 0. 1.7321 0. ]
[-1.5 -0.866 0. ]]

plots

new

old

[codecov]

Codecov Report

Merging #3710 (bf3e0e1) into main (846f183) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3710   +/-   ##
=======================================
  Coverage   94.04%   94.04%           
=======================================
  Files          83       83           
  Lines       18641    18641           
=======================================
  Hits        17530    17530           
  Misses       1111     1111           

[banesullivan]

Thank you for fixing this!

[tkoyama010]

LGTM. Thanks for fixing this.

[adeak]

Good catch on the issue, thanks @GlennWSo.

Just to muse a bit about the choice we have to make when fixing the bug: either keep the number of non-degenerate edges, or keep the number of actual edges. This PR chose the latter, which means that visually the result will change, and so will the point coordinates, but the size of the mesh will be the same. I'd be tempted to think that opting to keep the number of non-degenerate edges (i.e. sticking with a triangle for resolution=4) would have the potential to break downstream code relying on the exact size of the polygon (which would arguably be a good thing).

Anyway, the documentation of the helper clearly states that

    resolution : int, default: 100
        Number of points on the circle.

So the current PR's choice is a clear bug-fix that makes the function behave according to the API. Another question is whether this merits a [breaking-change] label... I'm only wondering about this because this will introduce subtle changes with no warning to end users.

[GlennWSo]

Good catch on the issue, thanks @GlennWSo.

Just to muse a bit about the choice we have to make when fixing the bug: either keep the number of non-degenerate edges, or keep the number of actual edges. This PR chose the latter, which means that visually the result will change, and so will the point coordinates, but the size of the mesh will be the same. I'd be tempted to think that opting to keep the number of non-degenerate edges (i.e. sticking with a triangle for resolution=4) would have the potential to break downstream code relying on the exact size of the polygon (which would arguably be a good thing).

Anyway, the documentation of the helper clearly states that

    resolution : int, default: 100
        Number of points on the circle.

So the current PR's choice is a clear bug-fix that makes the function behave according to the API. Another question is whether this merits a [breaking-change] label... I'm only wondering about this because this will introduce subtle changes with no warning to end users.

Agree, this change will brake some code. Im sure of this since this will brake some of my own code.

Maybe we can make a warning somehow? But im not sure how to best communicate this change to the end user.

[GlennWSo]

another point i want to make. With this change, pv.Circle will now be behave more like pv.Cylinder

[banesullivan]

My opinion: keep the changes as is but add a "breaking change" label to this PR so it shows up in the next release notes. We could add a note in the docstring that prior to version 0.38, this method had incorrect results, which are now fixed. I don't want to throw a warning though

[MatthewFlamm]

My opinion: keep the changes as is but add a "breaking change" label to this PR so it shows up in the next release notes. We could add a note in the docstring that prior to version 0.38, this method had incorrect results, which are now fixed. I don't want to throw a warning though

I agree and added the "breaking change" label. If @GlennWSo can add a section in the PR description describing the breaking change, then I think this meets the ability for users to find this change 'easily'. Then it is ready to merge IMO.

[GlennWSo]

My opinion: keep the changes as is but add a "breaking change" label to this PR so it shows up in the next release notes. We could add a note in the docstring that prior to version 0.38, this method had incorrect results, which are now fixed. I don't want to throw a warning though

I agree and added the "breaking change" label. If @GlennWSo can add a section in the PR description describing the breaking change, then I think this meets the ability for users to find this change 'easily'. Then it is ready to merge IMO.

Sure, i can do that and include an example.

[dcbr]

another point i want to make. With this change, pv.Circle will now be behave more like pv.Cylinder

This made me look through some other geometric objects and I found 2 others with wrong 'circular resolutions':

CylinderStructured

pyvista.CylinderStructured(theta_resolution=4).plot()

Ellipse

pyvista.Ellipse(resolution=4).plot()

I believe the fix for these is the same as for Circle, so probably a good idea to include in this PR as well.

[GlennWSo]

My opinion: keep the changes as is but add a "breaking change" label to this PR so it shows up in the next release notes. We could add a note in the docstring that prior to version 0.38, this method had incorrect results, which are now fixed. I don't want to throw a warning though

@banesullivan

Help wanted

im now struggling to pass the gh action build documentation.
Im not sure if it the rebase i recently made or the changes i made to the docstring that is causing problems.

[banesullivan]

@GlennWSo, the errors/warnings are burried deep in the build logs. These are what I initially saw that will need to be changed here:

/opt/hostedtoolcache/Python/3.9.15/x64/lib/python3.9/site-packages/numpydoc/docscrape.py:460: UserWarning: potentially wrong underline length... 
Notes 
-------- in 
Create a single PolyData circle defined by radius in the XY plane.
... in the docstring of Circle in /home/runner/work/pyvista/pyvista/pyvista/utilities/geometric_objects.py.
  warn(msg)

/home/runner/work/pyvista/pyvista/pyvista/utilities/geometric_objects.py:docstring of pyvista.utilities.geometric_objects.Circle:24: WARNING: Definition list ends without a blank line; unexpected unindent.
looking for now-outdated files... none found