FIX: update arc to do better rounding and allow wrap#29962
Conversation
jklymak
force-pushed
the
mnt-fix-arc
branch
2 times, most recently
from
ffdd0cf to
14d5441
Compare
dstansby
left a comment
dstansby
left a comment
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 |
There was a problem hiding this comment.
Docs are rewritten. Hopefully clearer
| # 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.
|
|
||
| @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?
There was a problem hiding this comment.
I don't see how to do that. I agree we should not have too many image tests, but sometimes they are clearer
There was a problem hiding this comment.
Instead of this one test, my suggestion is to have two tests:
- one which compares the code currently on ax[0] and ax[1] (because we expect them to be the same)
- one that compares the code currently on ax[2] to an empty axes (because we expect the code to not draw anything)
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
|
The original issue was closed by #31844, but there is still some useful work to the docstrings here. Do you want to rebase and merge those? |
3 participants
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.