FIX: update arc to do better rounding and allow wrap #29962
Conversation
There was a problem hiding this comment.
Looks good overall, but:
- Some code paths need test coverage
- I think the image test could compare two generated images, instead of being a full on figure test with a new test image
- Lots of stray
print()statements - I also left some suggestions for minor doc improvements
| (*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 |
There was a problem hiding this comment.
| (*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 | |
| (*theta2*) of the arc. If the arc spans more than 360 degrees, it | |
| will be wrapped to start at *theta1* and have a total extent <= 360 degrees, | |
| provided *wrap* is True. The arc is drawn counter-clockwise |
I think this is a bit clearer than talking about theta1 + 360?
| 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 |
There was a problem hiding this comment.
| 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 | |
| If True, the arc is wrapped to start at *theta1* and have an extent <= 360 degrees. | |
| If False, the arc is not wrapped. The arc will be |
| eta1, eta2 = np.deg2rad([eta1, eta2]) | ||
|
|
||
| # number of curve segments to make | ||
| if n is None: | ||
| n = int(2 ** np.ceil((eta2 - eta1) / halfpi)) | ||
| if np.abs(eta2 - eta1) <= 2.2 * np.pi: |
There was a problem hiding this comment.
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.
Does 2 * np.pi + 1e-3 or something similar work? Writing it like this makes it more obvious that the intention is 2pi + a tiny bit for numerical reasons.
| print('Here') | ||
| else: | ||
| # this will not grow exponentially if we allow wrapping arcs: | ||
| n = int(2 * np.ceil(2 * np.abs(eta2 - eta1) / np.pi)) |
There was a problem hiding this comment.
Looks like this is missing a test too.
| 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.
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.
I'm not sure how to make the requisite arcs without calling arc so I'm not sure what compare figure test would do? I could just make a full circle at exactly 360 degrees, but I'm not sure what that is testing.
There was a problem hiding this comment.
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[2] against an empty axes?
closes #29953
This actually fixes the numerical instability at least as in #29953. Also added the option,
*wrap*for larger arcs, but is optional, and the original behaviour is still default. Updated the docstring to be Numpy compatible.The new test shows that the new algorithm is a bit less fragile to numerical slop (presumably because
np.modis better with this, or at least we are standard compliant.Note that some tests tested that if theta2 < theta1 it stayed that way. So theta1=-90 and theta2=-110 has an arc that goes 320 degrees from -90 to +250, rather than one that goes from -90 to -110. eg the arc always goes counter clockwise. I've updated the docs to make this clear.