NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Update Ellipse position with ellipse.center#11057

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

timhoffm merged 2 commits intomatplotlib:masterfromeschutz:ellipse-center

Apr 16, 2018

Merged

Update Ellipse position with ellipse.center#11057

timhoffm merged 2 commits intomatplotlib:masterfromeschutz:ellipse-center

Apr 16, 2018

Conversation

Copy link

Contributor

PR Summary

Fixed an issue where Ellipses would not move whenellipse.center was set.

Example:

importmatplotlib.pyplotaspltfrommatplotlib.patchesimportEllipsedefonmove(event):ellipse.center=event.xdata,event.ydata# ellipse.stale = True # (old fix)# Ellipse position would not update after plt.draw()# Should move with mouseplt.draw()ellipse=Ellipse((1,1),1,1)plt.gca().add_patch(ellipse)plt.gca().set_aspect('equal')plt.gcf().canvas.mpl_connect('motion_notify_event',onmove)plt.xlim(0,4)plt.ylim(0,4)plt.show()

Old

New

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Added get/set_center to Ellipse

82e09ce

timhoffm reviewed

Apr 16, 2018

View reviewed changes

lib/matplotlib/patches.py Outdated

		@@ -1439,7 +1439,7 @@ def __init__(self, xy, width, height, angle=0.0, **kwargs):
		"""
		Patch.__init__(self, **kwargs)

		self.center = xy
		self.xy = xy

Copy link

Member

Choose a reason for hiding this comment

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

I'd useself._center instead ofself.xy. We shouldn't add a public attribute. Whilexy is used as a variable name in__init__, I don't think it's a good choice. "center" is more semantic.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Updated toself._center, thanks!

Changed Ellipse.xy to _center

134a0b6

dstansby approved these changes

Apr 16, 2018

View reviewed changes

Copy link

Member

ImportanceOfBeingErnest commentedApr 16, 2018

This for sure breaks a lot of existing code.Ellipse andCircle not having setters for the center position, usingCircle.center is the widely adopted way to change their position. One should not silently remove this attribute.
I suspect, being an attribute, it's somehow hard to deprecate via warnings issued; but I don't think this qualifies as a reason to let it disappear completely unnoticed from one version to next.

I personally do not quite see the reason to hide the attribute, but if people think this should be done, it should be done forall attributes of all patches and the respective setters and getters should be introduced.

Copy link

ContributorAuthor

eschutz commentedApr 16, 2018

TheEllipse.center attribute is still there, it’s just accessed through the getter and setter:center = property(get_center, set_center). This is the same wayCircle.radius is implemented.