Skip to content

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Jun 24, 2022

PR Summary

Now, the correct path is set for Arc which should solve the following.

image

I 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.arc on every redraw.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the arcfix branch 7 times, most recently from fe32d30 to 74bf6ae Compare June 24, 2022 14:24
@tacaswell tacaswell added this to the v3.6.0 milestone Jun 24, 2022
Copy link
Member

@tacaswell tacaswell left a 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.

@oscargus oscargus force-pushed the arcfix branch 2 times, most recently from 83cb036 to b3230fe Compare July 1, 2022 08:48
@oscargus
Copy link
Member Author

oscargus commented Jul 1, 2022

It seems like the added test also exercises the update-branch.

Edit: no, it is not.

@oscargus oscargus force-pushed the arcfix branch 2 times, most recently from 7739b7f to 63a048c Compare July 1, 2022 12:43
@oscargus oscargus marked this pull request as draft July 7, 2022 07:18
@oscargus oscargus force-pushed the arcfix branch 3 times, most recently from df52f01 to 6f4ae90 Compare August 4, 2022 12:08
@oscargus
Copy link
Member Author

oscargus commented Aug 4, 2022

This example fails without _update_path, so I will try to extract the relevant parts (probably it is enough to use units, although I do not really understand what changes the second time it is called...).

https://matplotlib.org/devdocs/gallery/units/ellipse_with_units.html

@oscargus oscargus force-pushed the arcfix branch 4 times, most recently from 1fd0bc7 to a651e73 Compare August 4, 2022 14:23
@oscargus
Copy link
Member Author

oscargus commented Aug 4, 2022

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.

@oscargus oscargus marked this pull request as ready for review August 4, 2022 15:07
@tacaswell
Copy link
Member

I realized that I had reused private names...

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(
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@anntzer anntzer left a 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.

@tacaswell tacaswell merged commit eca26d0 into matplotlib:main Aug 19, 2022
@oscargus oscargus deleted the arcfix branch August 19, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants