-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Set correct path for Arc #23340
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set correct path for Arc #23340
Conversation
fe32d30
to
74bf6ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
83cb036
to
b3230fe
Compare
It seems like the added test also exercises the update-branch. Edit: no, it is not. |
7739b7f
to
63a048c
Compare
df52f01
to
6f4ae90
Compare
This example fails without https://matplotlib.org/devdocs/gallery/units/ellipse_with_units.html |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in https://github.com/matplotlib/matplotlib/pull/22678/files#r831735789 you can get rid of the +360.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits, feel free to self-merge with or without fixing.
PR Summary
Now, the correct path is set for
Arc
which 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, 8000 for multiple draws there will not be a new creation of an
Path.arc
on every redraw.PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and 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).