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

Improve RectangleSelector rotation#21945

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

Draft
dstansby wants to merge5 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromdstansby:rect-rotation

Conversation

dstansby
Copy link
Member

@dstansbydstansby commentedDec 14, 2021
edited
Loading

PR Summary

This modifiesRectangleSelector andEllipseSelector to be drawn in display coordinates instead of display coordinates. This has a number of advantages:

  • Allows the rectangle to be rotated past the previous +/- 45 degrees limit
  • Fixes scaling the rectangle when it is rotated (to demonstrate, create a selector with rotation and try scaling before and then with this PR).
  • Removes the need for_aspect_ratio_correction.
  • Improves performance, because the event handling and artist drawing is all done in figure coordinates, removing transformations to data coordinates and back.

Code to test:

importmatplotlib.pyplotaspltfrommatplotlib.widgetsimportRectangleSelectorfig=plt.figure()ax=fig.add_subplot()selector=RectangleSelector(ax,lambda*args:None,interactive=True)selector.extents= (0.4,0.7,0.3,0.4)selector.add_state('rotate')plt.show()

Fixes#21937

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

@jklymakjklymak marked this pull request as draftDecember 14, 2021 12:19
@dstansbydstansbyforce-pushed therect-rotation branch 3 times, most recently fromb983b4e to4a86576CompareDecember 14, 2021 19:51
@dstansbydstansby changed the titleRect rotationImprove RectangleSelector rotationDec 15, 2021
@ericpre
Copy link
Member

This looks very promising!

@dstansby
Copy link
MemberAuthor

This is ready for review!

@jklymak
Copy link
Member

Confused about the status here - is it waiting, or is it ready? I'll assume waiting and set to draft, but feel free to pop back on queue.

@jklymakjklymak marked this pull request as draftJanuary 5, 2022 09:08
@ericpre
Copy link
Member

Best to sort out#21977 first before continuing on this PR.

@dhomeier
Copy link

I think it makes sense to keep the width/height fixed in display coords, and am undecided on whether a point (either the corner or center?) should be fixed in display or data coords.

Looking at it from the data side, so to speak, I would rather give preference to conserving area in data space – holding width and height fixed in data coords would of course be a sufficient, but perhaps not necessary condition for that.
Thinking in particular of the link toastropy/regions#390 here; I don't know if there are other applications in support of this, or if it really should get preference over implementing a better performing solution on the Matplotlib side.

@dstansby
Copy link
MemberAuthor

Looking at it from the data side, so to speak, I would rather give preference to conserving area in data space – holding width and height fixed in data coords would of course be a sufficient, but perhaps not necessary condition for that.

What are your thoughts in the instance where the selector is rotated, where it's impossible to keep the selector rectangular (ie. have right angles as corners in display coordinates)and conserve both width, height in data coordinates?

@dhomeier
Copy link

What are your thoughts in the instance where the selector is rotated, where it's impossible to keep the selector rectangular (ie. have right angles as corners in display coordinates)and conserve both width, height in data coordinates?

Intuitively it certainly makes interaction easier if angles and aspect ratio are conserved in display coordinates.
Just trying to figure out if the area in data coordinates is not automatically conserved after all – that certainly is the case for 45˚ or 90˚ rotations, as e.g. doubling one side in and aspect ratio of 2 would simply reduce the other by half.
And since the selector is no longer rectangular in data coordinates, the concept of width and height becomes ambiguous anyway – is it to be the parallelogram height or side length?

@jklymak
Copy link
Member

@dstansby have you considered making a simpler "RotatingRectangleSelector" that is mostly meant for the fixed-aspect ratio case? I really don't see the point of rotating a rectangle if the data dimensions are different in x and y, so this is really mostly for the image processing case?. If you spin it, it can have whatever whacky behaviour it wants if the data aspect is not 1:1 without introducing confusing behaviour in the normal RectangleSelector?

@dstansby
Copy link
MemberAuthor

I am not adverse to doing that, but as it currently stands in the main branch (from#20839) it is currently possible to rotate a rectangle with axes aspect!=1 and the rectangle stays rectangular in display coordinates, not data coordinates. This PR removes limitations on that approach and fixes bugs with it.

I am happy to either:

Either will take significant effort though, so it would be good to know which of these options has support! Personally I think it makes sense to have the rectangle always rectangular in display coords (this PR), but am not against creating a new selector to do this.

@jklymakjklymak added the status: needs comment/discussionneeds consensus on next step labelJun 23, 2022
@jklymak
Copy link
Member

@ericpre@anntzer both of your names come to mind to comment on how to proceed here...

I can't see ever using this, so I don't have a strong opinion about what is supposed to happen if we rotate a rectangle selector in non one-to-one aspect space. It seems ill defined. Maybe the solution is to not offer that option if the aspect ratio is not 1:1?

@jklymak
Copy link
Member

I added to the weekly meeting this week. However@dstansby if you would like to attend to discuss we can defer until you are available. Meetings are Thu 19:00 UTC :https://hackmd.io/49a-u44CTja02xQRaG88JA

@jklymak
Copy link
Member

We discussed on the meeting, and the consensus that the only sane thing is for the rectangle to stay a rectangle indisplay coordinates. Anything else doesn't work in any sort of non-cartesian co-ordinate system.

It also seems that resizing the window, etc is a bit beyond the point. If you make a selection hopefully something happens with that selection before any other GUI actions are made. I guess that depends not he use case, but it seems that GUI rectangles can't be expected to respond to window resizing, and anything built on the widget should deal with it another way.

@dstansby
Copy link
MemberAuthor

thanks for discussing! I'm afraid I don't have the time to dig this up at the moment, as you can probably see it's quite a complex PR!

@tacaswell
Copy link
Member

@dstansby What is the state / fate of this?

@dstansby
Copy link
MemberAuthor

I think the status is it should definitely go in, but the rebase needs handling, and the what's new and/or bugfixes probably need updating or adding.

I think one of@keflavich or@dhomeier might have some funding from astropy to get this into a mergeable state? Sorry if I'm wrong there, but if I'm right would one of you be happy picking this PR up?

@keflavich
Copy link
Contributor

There is funding in astropy to do this, but I can't put in the effort. Someone from Aperio, possibly@dhomeier, may be able to but we haven't worked out details yet.

@dhomeier
Copy link

I am planning to work on this along withastropy/regions#390, but certainly won't be able to start before next year.

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

@ericpreericpreericpre left review comments

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

Assignees
No one assigned
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

[ENH]: Remove +/- 45deg restriction on rotating RectangleSelector
8 participants
@dstansby@ericpre@jklymak@dhomeier@tacaswell@keflavich@QuLogic@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp