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

Clean up RectangleSelector move code#21921

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:mainfromdstansby:rect-sel-clean

Dec 12, 2021

Merged

Clean up RectangleSelector move code#21921

timhoffm merged 2 commits intomatplotlib:mainfromdstansby:rect-sel-clean

Dec 12, 2021

Conversation

Copy link

Member

dstansby commentedDec 10, 2021

PR Summary

I am working towards improvingRectangleSelector to allow rotation past +/- 45 degrees. This PR cleans up some of the movement code to:

Add a short docstring
Add thexy andxy_press variables to make the code more readable
Add a block of three lines at the top to determine whether to rotate, resize, or move
Use these variables in the if/else blocks below, to make it clear what is going on in each block

PR Checklist

Tests and Styling

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

Remove un-used variables

ff8195e

dstansby added the topic: widgets/UI label

Dec 10, 2021

dstansby requested a review fromericpre

December 10, 2021 19:29

dstansby added this to thev3.6.0 milestone

Dec 10, 2021

timhoffm reviewed

Dec 10, 2021

View reviewed changes

lib/matplotlib/widgets.py OutdatedShow resolvedHide resolved

lib/matplotlib/widgets.py Outdated

		resize = self._active_handle and not move

		if resize:
		# If resizing, use xy/xy_press in a de-rotated frame

Copy link

Member

Choose a reason for hiding this comment

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

Suggested change

	#If resizing,use xy/xy_press in a de-rotated frame
	# use xy/xy_press in a de-rotated frame

Optional: The code above aleady says it.

lib/matplotlib/widgets.py Outdated

		- Re-size
		- Contine the creation of a new shape
		"""
		xy = np.array([event.xdata, event.ydata])

Copy link

Member

Choose a reason for hiding this comment

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

Suggested change

	xy=np.array([event.xdata,event.ydata])
	xy=np.array([event.xdata,event.ydata])

Side note: I'm wondering if a small internal class_Point would make sense for such cases, basically namedtuple-like with arithmetics. So that we can do.

p = _Point(x, y)2*pp + (1, 2)p.x

Arithmetics should still be faster than the numpy overhead.

ericpre reviewed

Dec 11, 2021

View reviewed changes

Copy link

Member

ericpre left a comment

Choose a reason for hiding this comment

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

I am +-0 on this change, because even if it does make slightly more compact, I am not convinced that it makes the code more readable. To me this is a matter of taste and it doesn't really matter: I don't see it as an improvement or a regression!
Typically, for a matplotlib developerevent.xdata orevent.x has a meaning, while in case ofxy orxy_press, this is a redirection and one need to look up what it is.

An alternative would be to add some convenience properties (xy,xydata) to theEvent class. These properties could also support basic arithmetic, as suggested by@timhoffm inits comments.