Conversation
oscargus
force-pushed
the
arcfix
branch
7 times, most recently
from
fe32d30 to
74bf6ae
Compare
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
Including a test of updating an Arc would be better, but not going to block on that as this fixes 3 bugs and extends the test coverage as-is.
oscargus
force-pushed
the
arcfix
branch
2 times, most recently
from
83cb036 to
b3230fe
Compare
|
It seems like the added test also exercises the update-branch. Edit: no, it is not. |
oscargus
force-pushed
the
arcfix
branch
2 times, most recently
from
7739b7f to
63a048c
Compare
oscargus
force-pushed
the
arcfix
branch
3 times, most recently
from
df52f01 to
6f4ae90
Compare
|
This example fails without https://matplotlib.org/devdocs/gallery/units/ellipse_with_units.html |
oscargus
force-pushed
the
arcfix
branch
4 times, most recently
from
1fd0bc7 to
a651e73
Compare
|
Finally! While fighting the unit system, I realized that I had reused private names... But it was just to change them and everything worked! Reading the Patch-code a bit better, I also was able to trigger updates without units. |
I lost a couple of hours to the exact same issue working on the svg font fallback! |
| def _update_path(self): | ||
| # Compute new values and update and set new _path if any value changed | ||
| stretched = self._theta_stretch() | ||
| if any(a != b for a, b in zip( |
There was a problem hiding this comment.
I think that's just
if stretched != (self._theta1, self._theta2, ...): ...as it's a tuple comparison?
| y = np.sin(theta) | ||
| stheta = np.rad2deg(np.arctan2(scale * y, x)) | ||
| # arctan2 has the range [-pi, pi], we expect [0, 2*pi] | ||
| return (stheta + 360) % 360 |
There was a problem hiding this comment.
As noted in https://github.com/matplotlib/matplotlib/pull/22678/files#r831735789 you can get rid of the +360.
anntzer
left a comment
anntzer
left a comment
There was a problem hiding this comment.
Minor nits, feel free to self-merge with or without fixing.
4 participants
PR Summary
Now, the correct path is set for
Arcwhich should solve the following.plt.autoscale()fails for partialArc#23329I am still a bit doubtful if this will kill something else and so on, so I've not added tests yet.Also, not sure if this warrants a release note.There is a bit of inefficiency in the implementation as there are basically two paths created: one during draw time and one that is just stored (and used for e.g. 3D, scaling and collections). One the other hand, for multiple draws there will not be a new creation of an
Path.arcon every redraw.PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).