Skip to content

Simplify Rectangle and RegularPolygon. #18601

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

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 28, 2020

In Rectangle: no need to keep track and update of self._x1 and
self._x2 all the time, just when we compute the patch_transform
(which doesn't need to be split out into its own function, or stored
into an instance variable, given that it's never accessed via the
private attribute name).

In RegularPolygon: likewise, we can just compute the transform in
get_patch_transform(), and nowhere else, so the other attributes can
just be plain attributes instead of properties that update the transform
all the time.

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Is there any normal circumstance where creating these transforms is slow and actually worth caching?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 28, 2020

I guess caching may be nice but wouldn't worry about it unless someone shows that this is an actual bottleneck; also in any case this PR doesn't regress on that...

@QuLogic
Copy link
Member

QuLogic commented Sep 28, 2020

That's true for Rectangle, but not RegularPolygon? Also, something that RegularPolygon setter could have had, but appears to have been missed, is setting its stale property.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 28, 2020

RegularPolygon calls _update_transform before returning the patch_transform, so we're fine.
OTOH you are right about staleness...

@QuLogic
Copy link
Member

QuLogic commented Sep 28, 2020

Yes, but someone could call get_patch_transform and just hang onto that even after changing things. For example,

fig, ax = plt.subplots()

rp = mpatches.RegularPolygon((0, 2), 3)
ax.add_patch(rp)

ax.text(0, 0, 'Center', ha='center', va='center', transform=rp.get_transform())
rp.xy = (3, 5)

ax.set_xlim(-10, 10)
ax.set_ylim(-10, 10)

In Rectangle: no need to keep track and update of `self._x1` and
`self._x2` all the time, just when we compute the patch_transform
(which doesn't need to be split out into its own function, or stored
into an instance variable, given that it's never accessed via the
private attribute name).

In RegularPolygon: likewise, we can just compute the transform in
get_patch_transform(), and nowhere else, so the other attributes can
just be plain attributes instead of properties that update the transform
all the time.
@anntzer
Copy link
Contributor Author

anntzer commented Sep 29, 2020

Fair point re: RegularPolygon.get_patch_transform; fixed.

@QuLogic QuLogic merged commit fc18ccc into matplotlib:master Sep 29, 2020
@QuLogic QuLogic added this to the v3.4.0 milestone Sep 29, 2020
@anntzer anntzer deleted the patches branch September 29, 2020 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants