FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Set correct path for Arc by oscargus · Pull Request #23340 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Set correct path for Arc - #23340

Merged
tacaswell merged 1 commit into
matplotlib:mainfrom
oscargus:arcfix
Aug 19, 2022
ConversationCommits (1)ChecksFiles changed
Merged

Set correct path for Arc#23340
tacaswell merged 1 commit into
matplotlib:mainfrom
oscargus:arcfix

Conversation

oscargus commented Jun 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

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

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 force-pushed the arcfix branch 7 times, most recently from fe32d30 to 74bf6ae Compare June 24, 2022 14:24
tacaswell added this to the v3.6.0 milestone Jun 24, 2022
tacaswell reviewed Jun 24, 2022
View reviewed changes
Comment thread lib/matplotlib/patches.py Outdated
tacaswell reviewed Jun 24, 2022
View reviewed changes
Comment thread lib/matplotlib/patches.py Outdated
tacaswell mentioned this pull request Jun 24, 2022
Merged
6 tasks
tacaswell approved these changes Jun 24, 2022
View reviewed changes

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jklymak reviewed Jun 30, 2022
View reviewed changes
Comment thread lib/matplotlib/tests/test_patches.py Outdated
Comment thread lib/matplotlib/tests/test_patches.py Outdated
oscargus force-pushed the arcfix branch 2 times, most recently from 83cb036 to b3230fe Compare July 1, 2022 08:48

oscargus commented Jul 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 July 1, 2022 12:43
oscargus marked this pull request as draft July 7, 2022 07:18
oscargus force-pushed the arcfix branch 3 times, most recently from df52f01 to 6f4ae90 Compare August 4, 2022 12:08

oscargus commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

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 force-pushed the arcfix branch 4 times, most recently from 1fd0bc7 to a651e73 Compare August 4, 2022 14:23

oscargus commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

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 marked this pull request as ready for review August 4, 2022 15:07

tacaswell commented Aug 17, 2022

Copy link
Copy Markdown
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!

anntzer reviewed Aug 18, 2022
View reviewed changes
Comment thread lib/matplotlib/patches.py
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(

anntzer Aug 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think that's just

if stretched != (self._theta1, self._theta2, ...): ...

as it's a tuple comparison?

anntzer reviewed Aug 18, 2022
View reviewed changes
Comment thread lib/matplotlib/patches.py
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

anntzer Aug 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As noted in https://github.com/matplotlib/matplotlib/pull/22678/files#r831735789 you can get rid of the +360.

anntzer approved these changes Aug 18, 2022
View reviewed changes

anntzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Minor nits, feel free to self-merge with or without fixing.

tacaswell merged commit eca26d0 into matplotlib:main Aug 19, 2022
oscargus deleted the arcfix branch August 19, 2022 22:02
rschmehl mentioned this pull request Jan 11, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jklymak jklymak left review comments
tacaswell tacaswell approved these changes
anntzer anntzer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.6.0

4 participants

Back | FazBrowse Home | New Git URL