Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Merged

Conversation

ericpre
Copy link
Member

@ericpreericpre commentedAug 15, 2021
edited
Loading

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
  • Adddata_coordinates state to define if the square ratio should be calculated in data or figure coordinates
  • Add rotation ofRectangleSelector andEllipseSelector taking into account aspect ratio of the axes and using the center of the shape as rotation center.
importmatplotlib.pyplotaspltfrommatplotlib.widgetsimportRectangleSelectorimportnumpyasnpvalues=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 interactivelyselector.remove_state('rotate')# alternatively press 'r' keyselector.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 (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • 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).

keflavich reacted with thumbs up emoji
@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from86511fe to15fbb09CompareAugust 15, 2021 10:11
@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from0f2e907 to2a68321CompareAugust 18, 2021 16:34
@ericpre
Copy link
MemberAuthor

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

@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from2a68321 to0cb15cbCompareAugust 19, 2021 14:56
@ericpre
Copy link
MemberAuthor

Thanks@timhoffm, I addressed your comments.

@QuLogic
Copy link
Member

since this PR add selector rotation

Now the PR title needs updating.

@ericpreericpre changed the titleFix centre and square state of rectangle selector in interactive modeFix centre and square state and add rotation for rectangle selectorAug 20, 2021
@ericpre
Copy link
MemberAuthor

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

@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from0cb15cb to58bf650CompareAugust 25, 2021 08:56
@ericpre
Copy link
MemberAuthor

Rebase to fix the merge conflicts.

Copy link
Member

@dstansbydstansby left a comment

Choose a reason for hiding this comment

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

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

@ericpre
Copy link
MemberAuthor

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

dstansby reacted with thumbs up emoji

@jklymak
Copy link
Member

Moved to draft for now.

@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from3816a92 to03648c8CompareOctober 12, 2021 15:56
@ericpre
Copy link
MemberAuthor

Rebased and addressed@dstansby's review comments.

@ericpreericpre marked this pull request as ready for reviewOctober 12, 2021 18:18
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

The PR has grown to a size that is difficult to review. I did not have time to go through the changes inwidgets.py andtest_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?

@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch fromcdfd423 toc394be8CompareDecember 4, 2021 19:12
Copy link
MemberAuthor

@ericpreericpre left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Maybe better to

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

@ericpre
Copy link
MemberAuthor

Thanks@timhoffm, your comments should have been addressed in6a4abf3.


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
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
0 and 45°. The rotation is enabled ordisable by striking the *r* key
0 and 45°. The rotation is enabled ordisabled by striking the *r* key

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

dhomeier reacted with thumbs up emoji

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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 ofself._active_handle to be incorrect when rotated more than -+45, e.g.'W is picked while it should be aS, 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.

Choose a reason for hiding this comment

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

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 labelledSW could end up somewhere on theE 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.

@ericpreericpreforce-pushed thefix_centre_square_rectangle_selector branch from6a4abf3 to56710f5CompareDecember 7, 2021 00:01
@timhoffmtimhoffm added this to thev3.6.0 milestoneDec 7, 2021
@timhoffmtimhoffm merged commit21ea3fb intomatplotlib:mainDec 7, 2021
@timhoffm
Copy link
Member

Thanks@ericpre this is a useful addition!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dhomeierdhomeierdhomeier left review comments

@timhoffmtimhoffmtimhoffm approved these changes

@dstansbydstansbydstansby approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

7 participants
@ericpre@QuLogic@jklymak@timhoffm@tacaswell@dstansby@dhomeier

[8]ページ先頭

©2009-2025 Movatter.jp