Fix centre and square state and add rotation for rectangle selector #20839

ericpre · 2021-08-15T10:03:27Z

PR Summary

Fix centre and square state of rectangle selector in interactive mode. These states were working only when creating the selector.
Fix name coordinate handle: N and S were swapped
Add method to add a default state
Privatize state_modifier_keys
Add data_coordinates state to define if the square ratio should be calculated in data or figure coordinates
Add rotation of RectangleSelector and EllipseSelector taking into account aspect ratio of the axes and using the center of the shape as rotation center.

import matplotlib.pyplot as plt
from matplotlib.widgets import RectangleSelector
import numpy as np

values = np.arange(0, 100)

fig = plt.figure()
ax = fig.add_subplot()
ax.plot(values, values)

selector = RectangleSelector(ax, print, interactive=True,
                             drag_from_anywhere=True,
                             use_data_coordinates=True)
selector.add_state('rotate') # alternatively press 'r' key
# rotate the selector interactively

selector.remove_state('rotate') # alternatively press 'r' key

selector.add_state('square')

~~I would expect that this PR conflicts with #19864 and I would be interested to take over #19864 to rebase and fix the outstanding issues.~~ The selector rotation is added in this PR.

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 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).

ericpre · 2021-08-19T11:04:39Z

@dstansby, since this PR add selector rotation, would you be interested in having a look/reviewing it? The approach used in this PR is slightly different: support for rotation around around the centre is added to the Rectangle patch, which simplifies the changes and also match the behaviour of the ellipse patch.

It works with aspect ratio != 1 and the rotation is done around the centre. For the aspect ratio != 1, the default is to use figure coordinates - data coordinates can selected using the data_coordinates state.

lib/matplotlib/widgets.py

doc/api/next_api_changes/deprecations/20839-EP.rst

doc/users/next_whats_new/rotate_selector.rst

ericpre · 2021-08-19T18:11:24Z

Thanks @timhoffm, I addressed your comments.

QuLogic · 2021-08-20T00:20:26Z

since this PR add selector rotation

Now the PR title needs updating.

ericpre · 2021-08-20T07:47:54Z

Sorry, I update the description of this PR but forgot to update the title!

ericpre · 2021-08-25T08:56:45Z

Rebase to fix the merge conflicts.

dstansby

I haven't had time to go through everything, but left some comments and questions that are hopefully helpful!

doc/api/next_api_changes/deprecations/20839-EP.rst

doc/users/next_whats_new/rectangleselector_square_center.rst

lib/matplotlib/axes/_axes.py

lib/matplotlib/patches.py

lib/matplotlib/widgets.py

ericpre · 2021-09-13T17:29:28Z

Thanks @dstansby for the review, I shall come back to this PR beginning of Oct to address the comments.

jklymak · 2021-10-12T14:04:05Z

Moved to draft for now.

ericpre · 2021-10-12T18:18:44Z

Rebased and addressed @dstansby's review comments.

timhoffm

The PR has grown to a size that is difficult to review. I did not have time to go through the changes in widgets.py and test_widgets.py yet. Part of the problem is that the PR does many things (c.f. the bullet list in the first post).

Review effort scales super-linearly with PR size. So many smaller PRs are better than one large PR. @ericpre is there a chance you can extract some changes into small separate PRs to make review easier?

lib/matplotlib/patches.py

doc/users/next_whats_new/selector_improvement.rst

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

…angle patch

…gument

ericpre

Thanks @dstansby and @dhomeier for the review. All comments should have been addressed/replied.

doc/users/next_whats_new/selector_improvement.rst

lib/matplotlib/tests/test_widgets.py

lib/matplotlib/widgets.py

timhoffm · 2021-12-05T20:37:16Z

lib/matplotlib/widgets.py

+        if 'data_coordinates' in state:
+            refx, refy = dx, dy
+        else:
+            refx = event.xdata / (eventpress.xdata + 1e-6)


Are you sure 1e-6 is a universal solution? Can eventpress.xdata not become -1e-6?

Maybe better to

xdata_press = eventpress.xdata if eventpress.xdata != 0 else 1-e6 refx = event.xdata / xdata_press

lib/matplotlib/widgets.py

lib/matplotlib/patches.py

lib/matplotlib/widgets.py

ericpre · 2021-12-06T19:37:48Z

Thanks @timhoffm, your comments should have been addressed in 6a4abf3.

lib/matplotlib/widgets.py

timhoffm · 2021-12-06T23:15:21Z

doc/users/next_whats_new/selector_improvement.rst

+
+The `~matplotlib.widgets.RectangleSelector` and
+`~matplotlib.widgets.EllipseSelector` can now be rotated interactively between
+0 and 45°. The rotation is enabled or disable by striking the *r* key


Suggested change

0 and 45°. The rotation is enabled or disable by striking the *r* key

0 and 45°. The rotation is enabled or disabled by striking the *r* key

I think we should give a reason for these somewhat particular limits. Please add a sentence:

The range limits are currently dictated by the implementation.

or similar.

One a side note: Shouldn't it be possible to cover the range -45°...45°, at least there the corner order does not change. One only has to add a sign, which may be possible to infer from the mouse vs start_point location. But this does not have to be in this PR. It's already very big and almost ready to go in.

Good idea, I change to -45 to 45 because it is a easy change!

That would be great as it allowed at least to cover the entire range of orientations with some axis tricks!
Could you also add a note what specially was not working with larger angles, so this might be considered in a follow-up as discussed in #20839 (comment)?
In particular, did the problems occur only with specific backends? As I mentioned, I did not notice any serious problems with up to 90 (and in fact now tested up to ±180˚, which effectively allows any number of full rotations). The only obvious issue was, on subsequent resizing of the rotated rectangle, the centre seems to start moving on some kind of trochoid, but whole weird-looking behaviour on resize can of course be avoided by clicking to 'center' state.

Could you also add a note what specially was not working with larger angles, so this might be considered in a follow-up as discussed in #20839 (comment)?

I don't remember but I would expect the value of self._active_handle to be incorrect when rotated more than -+45, e.g. 'W is picked while it should be a S, etc.

The only obvious issue was, on subsequent resizing of the rotated rectangle, the centre seems to start moving on some kind of trochoid, but whole weird-looking behaviour on resize can of course be avoided by clicking to 'center' state.

I have noticed it recently and I think this is related to _get_aspect_ratio and it would be good to improve it in another PR.

I don't remember but I would expect the value of self._active_handle to be incorrect when rotated more than -+45, e.g. 'W is picked while it should be a S, etc.

Thanks; this will be a very useful starting point! Yes, I would expect that the point originally labelled SW could end up somewhere on the E side etc., but could not figure out where that would cause trouble. Any other hints what to watch out for with a follow-up are definitely appreciated.

The motion I think is inevitably somewhat confusing as the fix point for resizing is not rotated along with the bounding box, but that seems quite tricky to address. It is certainly recommendable to use 'center' mode for resizing of the rotated shapes.

lib/matplotlib/widgets.py

timhoffm · 2021-12-07T23:50:55Z

Thanks @ericpre this is a useful addition!

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 86511fe to 15fbb09 Compare August 15, 2021 10:11

QuLogic added the topic: widgets/UI label Aug 17, 2021

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 0f2e907 to 2a68321 Compare August 18, 2021 16:34

ericpre mentioned this pull request Aug 18, 2021

Allow Rectangle and Ellipse selectors to be rotated #19864

Closed

6 tasks

ericpre added the status: needs review label Aug 19, 2021

timhoffm reviewed Aug 19, 2021

View reviewed changes

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved

doc/api/next_api_changes/deprecations/20839-EP.rst Outdated Show resolved Hide resolved

doc/users/next_whats_new/rotate_selector.rst Outdated Show resolved Hide resolved

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 2a68321 to 0cb15cb Compare August 19, 2021 14:56

ericpre changed the title ~~Fix centre and square state of rectangle selector in interactive mode~~ Fix centre and square state and add rotation for rectangle selector Aug 20, 2021

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 0cb15cb to 58bf650 Compare August 25, 2021 08:56

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 58bf650 to 4102b08 Compare August 25, 2021 10:23

dhomeier mentioned this pull request Aug 26, 2021

Adding support for rotating mpl selectors of Ellipse,RectanglePixelRegion astropy/regions#390

Open

QuLogic added the status: needs rebase label Sep 1, 2021

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 4102b08 to 3816a92 Compare September 8, 2021 09:27

ericpre removed the status: needs rebase label Sep 8, 2021

dstansby reviewed Sep 13, 2021

View reviewed changes

dhomeier mentioned this pull request Sep 30, 2021

Add ability to rotate ROIs and ROISubsetState glue-viz/glue#2235

Merged

jklymak marked this pull request as draft October 12, 2021 14:03

jklymak removed the status: needs review label Oct 12, 2021

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 3816a92 to 03648c8 Compare October 12, 2021 15:56

ericpre marked this pull request as ready for review October 12, 2021 18:18

ericpre added the status: needs review label Oct 12, 2021

timhoffm reviewed Oct 13, 2021

View reviewed changes

lib/matplotlib/patches.py Outdated Show resolved Hide resolved

lib/matplotlib/patches.py Outdated Show resolved Hide resolved

lib/matplotlib/patches.py Outdated Show resolved Hide resolved

lib/matplotlib/patches.py Show resolved Hide resolved

dhomeier reviewed Nov 30, 2021

View reviewed changes

doc/users/next_whats_new/selector_improvement.rst Show resolved Hide resolved

dhomeier reviewed Nov 30, 2021

View reviewed changes

lib/matplotlib/tests/test_widgets.py Show resolved Hide resolved

dhomeier reviewed Dec 1, 2021

View reviewed changes

lib/matplotlib/widgets.py Show resolved Hide resolved

dhomeier mentioned this pull request Dec 4, 2021

Add interactive selector to Disk PV example keflavich/tutorials#1

Open

ericpre added 8 commits December 4, 2021 19:00

Fix shape rectangle when using rotation and the axes aspect ratio != 1

9349727

Add what's new entry and fix linting

7c47128

Improve documentation, docstring and comments

8546ca4

Make rectangle patch rotation point more generic

5b9acfe

Improve documentation

6e55fc8

Simplify state selectors and validate rotation_point attribute Rect…

5558585

…angle patch

Replace the data_coordinates state by the use_data_coordinates ar…

8ce27d0

…gument

Improve documentation

c394be8

ericpre force-pushed the fix_centre_square_rectangle_selector branch from cdfd423 to c394be8 Compare December 4, 2021 19:12

ericpre commented Dec 4, 2021

View reviewed changes

dstansby approved these changes Dec 5, 2021

View reviewed changes

timhoffm reviewed Dec 5, 2021

View reviewed changes

timhoffm reviewed Dec 6, 2021

View reviewed changes

Improve documentation and syntax

56710f5

ericpre force-pushed the fix_centre_square_rectangle_selector branch from 6a4abf3 to 56710f5 Compare December 7, 2021 00:01

timhoffm added this to the v3.6.0 milestone Dec 7, 2021

timhoffm approved these changes Dec 7, 2021

View reviewed changes

timhoffm removed the status: needs review label Dec 7, 2021

timhoffm merged commit 21ea3fb into matplotlib:main Dec 7, 2021

ericpre deleted the fix_centre_square_rectangle_selector branch December 8, 2021 18:47

ericpre mentioned this pull request Dec 13, 2021

Fix rotation selector for aspect ratio of the axes values different from 1 #21886

Draft

2 tasks

dstansby mentioned this pull request Dec 13, 2021

[ENH]: Remove +/- 45deg restriction on rotating RectangleSelector #21937

Open

dstansby mentioned this pull request Feb 7, 2022

Improve RectangleSelector rotation #21945

Draft

6 tasks

anntzer mentioned this pull request Nov 17, 2022

Use scatter for check boxes and set facecolors correctly in check boxes and radio buttons #24474

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix centre and square state and add rotation for rectangle selector #20839

Fix centre and square state and add rotation for rectangle selector #20839

ericpre commented Aug 15, 2021 •

edited

Loading

ericpre commented Aug 19, 2021

ericpre commented Aug 19, 2021

QuLogic commented Aug 20, 2021

ericpre commented Aug 20, 2021

ericpre commented Aug 25, 2021

dstansby left a comment

ericpre commented Sep 13, 2021

jklymak commented Oct 12, 2021

ericpre commented Oct 12, 2021

timhoffm left a comment

ericpre left a comment

timhoffm Dec 5, 2021

ericpre commented Dec 6, 2021

timhoffm Dec 6, 2021

timhoffm Dec 6, 2021

ericpre Dec 7, 2021

dhomeier Dec 7, 2021

ericpre Dec 7, 2021

dhomeier Dec 8, 2021

timhoffm commented Dec 7, 2021

	0 and 45°. The rotation is enabled or disable by striking the r key
	0 and 45°. The rotation is enabled or disabled by striking the r key

Fix centre and square state and add rotation for rectangle selector #20839

Fix centre and square state and add rotation for rectangle selector #20839

Conversation

ericpre commented Aug 15, 2021 • edited Loading

PR Summary

PR Checklist

ericpre commented Aug 19, 2021

ericpre commented Aug 19, 2021

QuLogic commented Aug 20, 2021

ericpre commented Aug 20, 2021

ericpre commented Aug 25, 2021

dstansby left a comment

Choose a reason for hiding this comment

ericpre commented Sep 13, 2021

jklymak commented Oct 12, 2021

ericpre commented Oct 12, 2021

timhoffm left a comment

Choose a reason for hiding this comment

ericpre left a comment

Choose a reason for hiding this comment

timhoffm Dec 5, 2021

Choose a reason for hiding this comment

ericpre commented Dec 6, 2021

timhoffm Dec 6, 2021

Choose a reason for hiding this comment

timhoffm Dec 6, 2021

Choose a reason for hiding this comment

ericpre Dec 7, 2021

Choose a reason for hiding this comment

dhomeier Dec 7, 2021

Choose a reason for hiding this comment

ericpre Dec 7, 2021

Choose a reason for hiding this comment

dhomeier Dec 8, 2021

Choose a reason for hiding this comment

timhoffm commented Dec 7, 2021

ericpre commented Aug 15, 2021 •

edited

Loading