Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: update arc to do better rounding and allow wrap#29962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -949,36 +949,70 @@ | ||||||||||||||
return cls._unit_circle_righthalf | ||||||||||||||
@classmethod | ||||||||||||||
def arc(cls, theta1, theta2, n=None, is_wedge=False, wrap=True): | ||||||||||||||
""" | ||||||||||||||
Return a `Path` fora counter-clockwiseunit circle arc from angles | ||||||||||||||
*theta1* to *theta2* (in degrees). | ||||||||||||||
Parameters | ||||||||||||||
---------- | ||||||||||||||
theta1, theta2 : float | ||||||||||||||
The angles (in degrees) defining the start (*theta1*) and end | ||||||||||||||
(*theta2*) of the arc. If the arc spans more than 360 degrees, it | ||||||||||||||
will be wrapped to fit within the range from *theta1* to *theta1* + | ||||||||||||||
360, provided *wrap* is True. The arc is drawn counter-clockwise | ||||||||||||||
Comment on lines +961 to +963 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Suggested change
I think this is a bit clearer than talking about | ||||||||||||||
from *theta1* to *theta2*. For instance, if *theta1* =90 and | ||||||||||||||
*theta2* = 70, the resulting arc will span 320 degrees. | ||||||||||||||
n : int, optional | ||||||||||||||
The number of spline segments to make. If not provided, the number | ||||||||||||||
of spline segments is determined based on the delta between | ||||||||||||||
*theta1* and *theta2*. | ||||||||||||||
is_wedge : bool, default: False | ||||||||||||||
If True, the arc is a wedge. The first vertex is the center of the | ||||||||||||||
wedge, the second vertex is the start of the arc, and the last | ||||||||||||||
vertex is the end of the arc. The wedge is closed with a line | ||||||||||||||
segment to the center of the wedge. If False, the arc is a | ||||||||||||||
polyline. The first vertex is the start of the arc, and the last | ||||||||||||||
vertex is the end of the arc. The arc is closed with a line | ||||||||||||||
segment to the start of the arc. The wedge is not closed with a | ||||||||||||||
line segment to the start of the arc. | ||||||||||||||
wrap : bool, default: True | ||||||||||||||
If True, the arc is wrapped to fit between *theta1* and *theta1* + | ||||||||||||||
360 degrees. If False, the arc is not wrapped. The arc will be | ||||||||||||||
Comment on lines +983 to +984 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Suggested change
| ||||||||||||||
drawn from *theta1* to *theta2*. | ||||||||||||||
Notes | ||||||||||||||
----- | ||||||||||||||
The arc is approximated using cubic Bézier curves, as described in | ||||||||||||||
Masionobe, L. 2003. `Drawing an elliptical arc using polylines, | ||||||||||||||
quadratic or cubic Bezier curves | ||||||||||||||
<https://web.archive.org/web/20190318044212/http://www.spaceroots.org/documents/ellipse/index.html>`_. | ||||||||||||||
""" | ||||||||||||||
eta1 = theta1 | ||||||||||||||
if wrap: | ||||||||||||||
# Wrap theta2 to 0-360 degrees from theta1. | ||||||||||||||
eta2 = np.mod(theta2 - theta1, 360.0) + theta1 | ||||||||||||||
# Ensure 360-deg range is not flattened to 0 due to floating-point | ||||||||||||||
# errors, but don't try to expand existing 0 range. | ||||||||||||||
if theta2 != theta1 and eta2 <= eta1: | ||||||||||||||
eta2 += 360 | ||||||||||||||
else: | ||||||||||||||
eta2 = theta2 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Looks like this new case (wrap=False) needs a test. Probably not a figure test, but something that checks that an arc is drawn with an extent > 360 deg would be good. | ||||||||||||||
eta1, eta2 = np.deg2rad([eta1, eta2]) | ||||||||||||||
# number of curve segments to make | ||||||||||||||
if n is None: | ||||||||||||||
if np.abs(eta2 - eta1) <= 2.2 * np.pi: | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Why 2.2pi here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. If it's exactly 2pi some tests fail. 2.2 pi doesn't really matter because it deals with pi/2 increments. I could make it 2.25 pi? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Does | ||||||||||||||
# this doesn't need to grow exponentially, but we have left | ||||||||||||||
# this way for back compatibility | ||||||||||||||
n = int(2 ** np.ceil(2 * np.abs(eta2 - eta1) / np.pi)) | ||||||||||||||
else: | ||||||||||||||
# this will grow linearly if we allow wrapping arcs: | ||||||||||||||
n = int(2 * np.ceil(2 * np.abs(eta2 - eta1) / np.pi)) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Looks like this is missing a test too. | ||||||||||||||
if n < 1: | ||||||||||||||
raise ValueError("n must be >= 1 or None") | ||||||||||||||
@@ -1028,22 +1062,32 @@ | ||||||||||||||
return cls(vertices, codes, readonly=True) | ||||||||||||||
@classmethod | ||||||||||||||
def wedge(cls, theta1, theta2, n=None, wrap=True): | ||||||||||||||
""" | ||||||||||||||
Return a `Path` for a counter-clockwise unit circle wedge from angles | ||||||||||||||
*theta1* to *theta2* (in degrees). | ||||||||||||||
Parameters | ||||||||||||||
---------- | ||||||||||||||
theta1, theta2 : float | ||||||||||||||
The angles (in degrees) defining the start (*theta1*) and end | ||||||||||||||
(*theta2*) of the arc. If the arc spans more than 360 degrees, it | ||||||||||||||
will be wrapped to fit within the range from *theta1* to *theta1* + | ||||||||||||||
360, provided *wrap* is True. The arc is drawn counter-clockwise | ||||||||||||||
from *theta1* to *theta2*. For instance, if *theta1* =90 and | ||||||||||||||
*theta2* = 70, the resulting arc will span 320 degrees. | ||||||||||||||
n : int, optional | ||||||||||||||
The number of spline segments to make. If not provided, the number | ||||||||||||||
of spline segments is determined based on the delta between | ||||||||||||||
*theta1* and *theta2*. | ||||||||||||||
wrap : bool, default: True | ||||||||||||||
If True, the arc is wrapped to fit between *theta1* and *theta1* + | ||||||||||||||
360 degrees. If False, the arc is not wrapped. The arc will be | ||||||||||||||
drawn from *theta1* to *theta2*. | ||||||||||||||
""" | ||||||||||||||
return cls.arc(theta1, theta2, n, wedge=True, wrap=wrap) | ||||||||||||||
@staticmethod | ||||||||||||||
@lru_cache(8) | ||||||||||||||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -471,12 +471,49 @@ def test_full_arc(offset): | ||
high = 360 + offset | ||
path = Path.arc(low, high) | ||
print(path.vertices) | ||
jklymak marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
mins = np.min(path.vertices, axis=0) | ||
maxs = np.max(path.vertices, axis=0) | ||
np.testing.assert_allclose(mins, -1) | ||
np.testing.assert_allclose(maxs, 1) | ||
@image_comparison(['arc_close360'], style='default', remove_text=True, | ||
extensions=['png']) | ||
def test_arc_close360(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Can you do this as a compare images test, instead of a figure test that adds a new figure? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I'm not sure how to make the requisite arcs without calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. For this test, can you compare what you're generating on ax[0] and ax[1] against each other, and then compare what you're plotting on | ||
_, ax = plt.subplots(ncols=3) | ||
ax[0].add_patch(patches.PathPatch(Path.arc(theta1=-90 - 1e-14, theta2=270))) | ||
#ax[0].set_title("arc(-90-1e-14, 270), should be a circle") | ||
ax[1].add_patch(patches.PathPatch(Path.arc(theta1=-90, theta2=270))) | ||
#ax[1].set_title("arc(-90, 270), is a circle") | ||
ax[2].add_patch(patches.PathPatch(Path.arc(theta1=-90 - 1e-14, theta2=-90))) | ||
#ax[2].set_title("arc(-90, -90-1e-14), should not be a circle") | ||
for a in ax: | ||
a.set_xlim(-1, 1) | ||
a.set_ylim(-1, 1) | ||
a.set_aspect("equal") | ||
@image_comparison(['arc_wrap_false'], style='default', remove_text=True, | ||
extensions=['png']) | ||
def test_arc_wrap_false(): | ||
_, ax = plt.subplots(2, 2) | ||
ax = ax.flatten() | ||
ax[0].add_patch(patches.PathPatch(Path.arc(theta1=10, theta2=20, | ||
is_wedge=True, wrap=True))) | ||
ax[1].add_patch(patches.PathPatch(Path.arc(theta1=10, theta2=380, | ||
is_wedge=True, wrap=True))) | ||
ax[2].add_patch(patches.PathPatch(Path.arc(theta1=10, theta2=20, | ||
is_wedge=True, wrap=False))) | ||
ax[3].add_patch(patches.PathPatch(Path.arc(theta1=10, theta2=740, | ||
is_wedge=True, wrap=False))) | ||
for a in ax: | ||
a.set_xlim(-1, 1) | ||
a.set_ylim(-1, 1) | ||
a.set_aspect("equal") | ||
def test_disjoint_zero_length_segment(): | ||
this_path = Path( | ||
np.array([ | ||
Uh oh!
There was an error while loading.Please reload this page.