Repository: pyvista/pyvista

Issue:
circle creates creates one zero length edge
### Describe the bug, what's wrong, and what you expected.

I expected that:
> circle = pv.Circle(radius, resolution=n)

1. would create a circle with n points and edge. 
It does yay :-) 

3. That each edge of the would have similar length.
It does _not_ :-(

The problems seems circle  closed is doubly:
- once by the coordinates (circle.points[:, 0] == approx(circle.points[:, -1])
- and a second time by the face (circle.faces[0] == [n, 0, 1, 2, ... n-1])





### Steps to reproduce the bug.

```python
import pyvista as pv

circle = pv.Circle(radius=1, resolution=4) # lets make a low res circle
print(circle.faces)  # out: array([4, 0, 1, 2, 3])
print(circle.n_points)  # out: 4

# the print outputs gives the expectation that circle.plot() will look like a square 
circle.plot()
```
![pv.Circle(radius=1, resolution=4).plot()](https://user-images.githubusercontent.com/107837123/207049939-9b24ac31-a8e8-4ca7-97d3-3f4105a524dc.png)




### System Information

```shell
--------------------------------------------------------------------------------
  Date: Mon Dec 12 13:55:16 2022 CET

                OS : Linux
            CPU(s) : 8
           Machine : x86_64
      Architecture : 64bit
       Environment : IPython
       GPU Details : error

  Python 3.10.4 (main, Mar 23 2022, 20:25:24) [GCC 11.3.0]

           pyvista : 0.36.1
               vtk : 9.1.0
             numpy : 1.23.5
           imageio : 2.22.4
           appdirs : 1.4.4
            scooby : 0.7.0
        matplotlib : 3.6.2
         pyvistaqt : 0.9.0
             PyQt5 : Version unknown
           IPython : 8.2.0
             scipy : 1.9.3
--------------------------------------------------------------------------------
```


### Screenshots

_No response_

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/pyvista/pyvista/pull/3710?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvista) Report
> Merging [#3710](https://codecov.io/gh/pyvista/pyvista/pull/3710?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvista) (f3d4467) into [main](https://codecov.io/gh/pyvista/pyvista/commit/ffa9b4ceb9e299c429c5a45e820820b868aa440a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvista) (ffa9b4c) will **not change** coverage.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##             main    #3710   +/-   ##
=======================================
  Coverage   94.05%   94.05%           
=======================================
  Files          83       83           
  Lines       18558    18558           
=======================================
  Hits        17454    17454           
  Misses       1104     1104           
```

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 of 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.

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
![cylinder4](https://user-images.githubusercontent.com/107837123/207282017-2d402809-31e7-48dc-8f54-2c6b6aa64c90.png)

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':

<details><summary>CylinderStructured</summary>

```py
pyvista.CylinderStructured(theta_resolution=4).plot()
```
![image](https://user-images.githubusercontent.com/15089458/207549389-2ab7ac02-6f16-4931-9cd1-4b96a600afca.png)

</details>

<details><summary>Ellipse</summary>

```py
pyvista.Ellipse(resolution=4).plot()
```
![image](https://user-images.githubusercontent.com/15089458/207550247-6d86dd74-b0be-41ab-b01c-5424b9e517ef.png)

</details>

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
```

Patch:
diff --git a/pyvista/demos/demos.py b/pyvista/demos/demos.py
index fc576eb1..baaf1d57 100644
--- a/pyvista/demos/demos.py
+++ b/pyvista/demos/demos.py
@@ -15,7 +15,7 @@ def glyphs(grid_sz=3):
 
     Parameters
     ----------
-    grid_sz : int, optional
+    grid_sz : int, default: 3
         Create ``grid_sz x grid_sz`` supertoroids.
 
     Returns
@@ -63,7 +63,7 @@ def plot_glyphs(grid_sz=3, **kwargs):
 
     Parameters
     ----------
-    grid_sz : int, optional
+    grid_sz : int, default: 3
         Create ``grid_sz x grid_sz`` supertoroids.
 
     **kwargs : dict, optional
@@ -212,18 +212,18 @@ def plot_wave(fps=30, frequency=1, wavetime=3, interactive=False, notebook=None)
 
     Parameters
     ----------
-    fps : int, optional
-        Maximum frames per second to display.  Defaults to 30.
+    fps : int, default: 30
+        Maximum frames per second to display.
 
-    frequency : float, optional
-        Wave cycles per second.  Defaults to 1 Hz.
+    frequency : float, default: 1.0
+        Wave cycles per second (Hz).
 
-    wavetime : float, optional
-        The desired total display time in seconds.  Defaults to 3 seconds.
+    wavetime : float, default: 3.0
+        The desired total display time in seconds.
 
-    interactive : bool, optional
+    interactive : bool, default: False
         Allows the user to set the camera position before the start of the
-        wave movement.  Default ``False``.
+        wave movement.
 
     notebook : bool, optional
         When ``True``, the resulting plot is placed inline a jupyter
diff --git a/pyvista/utilities/geometric_objects.py b/pyvista/utilities/geometric_objects.py
index 170fef4d..ed02c576 100644
--- a/pyvista/utilities/geometric_objects.py
+++ b/pyvista/utilities/geometric_objects.py
@@ -1282,15 +1282,22 @@ def Circle(radius=0.5, resolution=100):
     pyvista.PolyData
         Circle mesh.
 
+    Notes
+    -----
+    .. versionchanged:: 0.38.0
+       Prior to version 0.38, this method had incorrect results, producing
+       inconsistent edge lengths and a duplicated point which is now fixed.
+
     Examples
     --------
     >>> import pyvista
     >>> radius = 0.5
     >>> circle = pyvista.Circle(radius)
     >>> circle.plot(show_edges=True, line_width=5)
+
     """
     points = np.zeros((resolution, 3))
-    theta = np.linspace(0.0, 2.0 * np.pi, resolution)
+    theta = np.linspace(0.0, 2.0 * np.pi, resolution, endpoint=False)
     points[:, 0] = radius * np.cos(theta)
     points[:, 1] = radius * np.sin(theta)
     cells = np.array([np.append(np.array([resolution]), np.arange(resolution))])


Test patch:
diff --git a/tests/test_geometric_objects.py b/tests/test_geometric_objects.py
index 12b5ff68..80ae189c 100644
--- a/tests/test_geometric_objects.py
+++ b/tests/test_geometric_objects.py
@@ -275,6 +275,11 @@ def test_circle():
     assert mesh.n_cells
     diameter = np.max(mesh.points[:, 0]) - np.min(mesh.points[:, 0])
     assert np.isclose(diameter, radius * 2.0, rtol=1e-3)
+    line_lengths = np.linalg.norm(
+        np.roll(mesh.points, shift=1, axis=0) - mesh.points,
+        axis=1,
+    )
+    assert np.allclose(line_lengths[0], line_lengths)
 
 
 def test_ellipse():