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

Improve RectangleSelector Rotation revisited and rebased #26833

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

Conversation

dhomeier
Copy link

@dhomeier dhomeier commented Sep 19, 2023

PR summary

This PR rebases the changes from #21945 onto (post-3.8.0) main. Since the merge conflicts after almost 2 years became prohibitive, I have applied the original modifications to a fresh PR. Almost all actual code changes are taken from @dstansby's original PR; from its summary:

This modifies RectangleSelector and EllipseSelector 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:

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


fig = 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()

This implementation preserves shapes in display coordinates as discussed in #21945 (comment)
Renamed a property _geometry_state in response to #21945 (comment)

Last (new) commit to fix MatplotlibDeprecationWarning: Setting data with a non sequence... raised by calling self._center_handle.set_data(*self.center) on a single coordinate array.
The workaround does not feel very pretty; might discuss whether self.center should instead return a tuple (np.array[xc], np.array[yc]) in


but this would in turn break tests like assert_allclose(tool.center, (50, 65)).

Closes #21937

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.


import pytest


Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for rebasing this!

If you add this blank line back, the now failing tests should pass.

@ksunden
Copy link
Member

ksunden commented Sep 20, 2023

Tests were failed due to (now yanked) setuptools-scm v8.0.0, restarting tests

@ksunden ksunden closed this Sep 20, 2023
@ksunden ksunden reopened this Sep 20, 2023
@dhomeier
Copy link
Author

dhomeier commented Sep 20, 2023

Thanks! I ran into the same issue managing to fetch 8.0.0 in the single hour it was online at condaforge... 😬

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

just the spelling things identified by pre-commit, Haven't looked in detail at the actual code changes yet

We are still having problems with setuptools-scm 8.0.1, though less severe than before, seeming to only really impact Azure 3.9 builds, documented in #26846

ymin, ymax = sorted([y0, y1])
def _clip_to_axes(self, x, y):
"""
Clip x and y values in diplay coordinates to the limits of the current
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
Clip x and y values in diplay coordinates to the limits of the current
Clip x and y values in display coordinates to the limits of the current

x0 += width - new_wh
width = height = new_wh

# Transform back into de-rotated display coordiantes
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
# Transform back into de-rotated display coordiantes
# Transform back into de-rotated display coordinates

@story645
Copy link
Member

@dhomeier this looks almost good to go, are you planning on coming back to this?

@astrofrog
Copy link
Contributor

This currently has an issue with the y limits of the selector 'snapping' to the bottom/x axis - to adapt an example @dhomeier shared with me:

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

data = np.random.random((16, 16))
plt.ion()
fig, ax = plt.subplots()
ax.imshow(data)
selector = RectangleSelector(ax, lambda *args: None, interactive=True)
selector.extents = (5, 10, 6, 8)
plt.show(block=True)

The following happens:

Screencast.from.19-07-24.10.15.14.webm

For some reason the recording doesn't show the cursor but basically I first click on the lower right selection and move it slightly but it snaps to the bottom, then I touch the top right selector and the same thing happens.

@astrofrog
Copy link
Contributor

@dhomeier - I've fixed the issue mentioned above in this PR to your branch: dhomeier#13

@astrofrog
Copy link
Contributor

astrofrog commented Jul 19, 2024

Another issue I ran into when testing this is that the region does not correctly follow the data when resizing the figure:

Screencast.from.19-07-24.10.32.46.webm

@dhomeier
Copy link
Author

Thanks, seems I never properly set that example up with matplotlib on its own, but in astropy/regions#390 those operations are failing with a "negative height" set to the move or resized rectangle, where height was set from selector.extents[3] -selector.extents[2] (which have snapped to something like (5.2, 10.2, 14.4, 16.4) and then (5.2, 10.2, 15.4, 16.4) in the above example.

dhomeier and others added 5 commits July 19, 2024 11:40
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
@dhomeier
Copy link
Author

@dhomeier - I've fixed the issue mentioned above in this PR to your branch: dhomeier#13

Thanks, that does fix the original regions problem as well – no idea how I could not have looked at that!
Also still seeing some issues with the outline being offset.

@astrofrog
Copy link
Contributor

Ok so the issue in #26833 (comment) is a little tricker as far as I can see - as the selector is defined in display coordinates, it does not follow the data when resizing the figure. We have to somehow cache the position of the selector in data coordinates so that we can change it in _on_resize. I'm not sure what the best way is to do this though, I've tried a couple of simple approaches which didn't work, I think the tricky bit will be dealing with the case with non-zero rotation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

[ENH]: Remove +/- 45deg restriction on rotating RectangleSelector
6 participants