The Wayback Machine - https://web.archive.org/web/20211024154637/https://github.com/matplotlib/matplotlib/pull/20839
Skip to content
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

Jump to bottom

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 16 commits into from

Conversation

@ericpre
Copy link
Member

@ericpre ericpre commented Aug 15, 2021 •
edited

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)
selector.add_default_state('center')
selector.add_default_state('square')

# change the size of selector interactively

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 ericpre force-pushed the fix_centre_square_rectangle_selector branch from 86511fe to 15fbb09 Aug 15, 2021
@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 0f2e907 to 2a68321 Aug 18, 2021
@ericpre ericpre mentioned this pull request Aug 18, 2021
Draft
6 tasks
@ericpre
Copy link
Member Author

@ericpre ericpre commented Aug 19, 2021

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

timhoffm
timhoffm reviewed Aug 19, 2021
View 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 ericpre force-pushed the fix_centre_square_rectangle_selector branch from 2a68321 to 0cb15cb Aug 19, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Aug 19, 2021

Thanks @timhoffm, I addressed your comments.

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Aug 20, 2021

since this PR add selector rotation

Now the PR title needs updating.

@ericpre 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
Copy link
Member Author

@ericpre ericpre commented Aug 20, 2021

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

@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 0cb15cb to 58bf650 Aug 25, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Aug 25, 2021

Rebase to fix the merge conflicts.

@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 58bf650 to 4102b08 Aug 25, 2021
@dhomeier dhomeier mentioned this pull request Aug 26, 2021
Open
@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 4102b08 to 3816a92 Sep 8, 2021
dstansby
dstansby reviewed Sep 13, 2021
View changes
Copy link
Member

@dstansby dstansby left a comment

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 Outdated Show resolved Hide resolved
doc/users/next_whats_new/rectangleselector_square_center.rst Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/patches.py Show resolved Hide resolved
lib/matplotlib/widgets.py
- "center": Make the initial point the center of the shape,
default: "ctrl".
- "center": change the shape around its center, default: "ctrl".
- "data_coordinates": define if data or figure coordinates should be
Copy link
Member

@dstansby dstansby Sep 13, 2021

It seems weird to have this as part of the state that can change. I think it would be better as a property that is fixed once the selector is created.

Copy link
Member Author

@ericpre ericpre Sep 13, 2021

Yes, I agree that it doesn't feel very natural. I guess that I have done it that way because of the it follow a similar pattern as rotate/center and it is useful for testing or debugging purpose, but there are not really good reasons! :)
I am happy to have it as a property.

@ericpre
Copy link
Member Author

@ericpre ericpre commented Sep 13, 2021

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

@dhomeier dhomeier mentioned this pull request Sep 30, 2021
Open
@jklymak jklymak marked this pull request as draft Oct 12, 2021
@jklymak
Copy link
Member

@jklymak jklymak commented Oct 12, 2021

Moved to draft for now.

@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 3816a92 to 03648c8 Oct 12, 2021
@ericpre
Copy link
Member Author

@ericpre ericpre commented Oct 12, 2021

Rebased and addressed @dstansby's review comments.

@ericpre ericpre marked this pull request as ready for review Oct 12, 2021
timhoffm
timhoffm reviewed Oct 13, 2021
View changes
Copy link
Member

@timhoffm timhoffm left a comment

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 Outdated
@@ -706,7 +706,8 @@ def __str__(self):
return fmt % pars

@docstring.dedent_interpd
def __init__(self, xy, width, height, angle=0.0, **kwargs):
def __init__(self, xy, width, height, angle=0.0,
Copy link
Member

@timhoffm timhoffm Oct 13, 2021

Suggested change
def __init__(self, xy, width, height, angle=0.0,
def __init__(self, xy, width, height, angle=0.0, *

Let's be defensive and make the new parameter keyword-only.

lib/matplotlib/patches.py Outdated
Rotation in degrees anti-clockwise about *xy*.
Rotation in degrees anti-clockwise about *xy* if
*rotate_around_center* if False, otherwise rotate around the
center of the rectangle
Copy link
Member

@timhoffm timhoffm Oct 13, 2021

Suggested change
center of the rectangle
center of the rectangle.

lib/matplotlib/patches.py Outdated
Rotation in degrees anti-clockwise about *xy* if
*rotate_around_center* if False, otherwise rotate around the
center of the rectangle
rotate_around_center : bool, default: False
Copy link
Member

@timhoffm timhoffm Oct 13, 2021

Suggested change
rotate_around_center : bool, default: False
rotation_point : {'xy', 'center'}, default: 'xy'

Is a better API because it leaves the possibility for other rotation points open.

lib/matplotlib/patches.py
@@ -1511,6 +1533,8 @@ def __init__(self, xy, width, height, angle=0, **kwargs):
self._width, self._height = width, height
self._angle = angle
self._path = Path.unit_circle()
# Required for EllipseSelector with axes aspect ratio != 1
self._aspect_ratio_correction = 1.0
Copy link
Member

@timhoffm timhoffm Oct 13, 2021

Please also describe the meaning of the parameter in the comment.

@ericpre
Copy link
Member Author

@ericpre ericpre commented Oct 14, 2021

@timhoffm, I fully appreciate that the review effort of this PR are not small and I kept the git history very tidy so that it is easier to understand the logic. Fixing centre and square state correctly was the difficult part and required quite a few iterations on my side (not reflected in this PR) but it made adding support for rotation easy and this is why I finally added in this PR even if this wasn't an original aim of this PR.
When working on this PR, there was quite a bit of iteration, which is not visible here and I suspect that if I would have done in separate PRs, this would have polluted the git history. Even if this PR is large, all changes are related and the git history was written in a way that it should be useful for future references.

One part that I could split out of this PR is the changes on the patches - the part that you started reviewing. This part is fairly simple and I suspect the difficult part of this PR can't be split easily or it could end up being unclear or difficult to appreciate the context and motivation. Overall, I am under the impression that that the gain will be marginal if I split in more than 2 PRs.
@timhoffm, do you want me to split the patches changes out of this PR? Is there something else you think would be useful to split out?
Maybe an alternative would be to open a dummy PR to a specific commit to be able to use the github compare changes to cover the "difficult" part - for example up to f63bd60?

@timhoffm
Copy link
Member

@timhoffm timhoffm commented Oct 15, 2021

@ericpre Thanks for your understanding. I don't have a complete overview of the changes and thus cannot really judge whether splitting is possible. If you can move some aspects out, that would certainly help. I trust your jugement what is reasonable and what is not.

Maybe an alternative would be to open a dummy PR to a specific commit to be able to use the github compare changes to cover the "difficult" part - for example up to f63bd60?

Sorry, I couldn't follow that. One can filter on commits in the "Files changed" tab. Is the proposal something different?

@ericpre
Copy link
Member Author

@ericpre ericpre commented Oct 16, 2021

Maybe an alternative would be to open a dummy PR to a specific commit to be able to use the github compare changes to cover the "difficult" part - for example up to f63bd60?

Sorry, I couldn't follow that. One can filter on commits in the "Files changed" tab. Is the proposal something different?

No, it would be the same but the github functionality will be the much more convenient to use - I wasn't aware of this functionality!
Since that the git history is in good shape, I think that the best is to filter commits in the "Files changed" tab and go through a few commits at a time.

@tacaswell
Copy link
Member

@tacaswell tacaswell commented Oct 20, 2021

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

ericpre added 16 commits Oct 20, 2021
@ericpre
Fix name coordinate handle: N and S were swapped
9f9d5cc
@ericpre
Fix center modifier in interactive rectangle selector - in _onmove of…
7772561 
… existing shape
@ericpre
Fix square modifier in interactive rectangle selector - in _onmove of…
537dc13 
… existing shape, part 1, with centering on
@ericpre
Fix square modifier in interactive rectangle selector - in _onmove of…
52e1a07 
… existing shape, part 2, with centering off
@ericpre
Add method to add default state
f1c9041
@ericpre
Privatize state_modifier_keys
2fe3cb2
@ericpre
Take into account aspect ratio in square state
bc3d3bc
@ericpre
Add what's new entry
fd26cee
@ericpre
Implement rotation selector
55489d7
@ericpre
Apply inverse transformation to event so that the onmove calculation …
8120ca6 
…are correct
@ericpre
Fix shape rectangle when using rotation and the axes aspect ratio != 1
a8a9487
@ericpre
Add what's new entry and fix linting
b8d2bec
@ericpre
Improve documentation, docstring and comments
540fffd
@ericpre
Make rectangle patch rotation point more generic
de8dfa9
@ericpre
Add Line2DSelector
597d08d
@ericpre
Add width handle and fix incorrect shape when using axis with aspect …
c7a7a83 
…ratio != 1
@ericpre ericpre force-pushed the fix_centre_square_rectangle_selector branch from 1502ef5 to c7a7a83 Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm

@dstansby dstansby

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: needs review topic: widgets/UI
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@ericpre @QuLogic @jklymak @timhoffm @tacaswell @dstansby