Thanks for the review - I marked all the minor changes as resolved, and have addressed the other points too.

Still needs a test of resizing after rotation.

Thanks for catching that, it revealed a (now fixed) bug in how I was calculating the resizing. I've also paramatrized the test so it runs for bothRectangle andEllipse selectors.

OK, I tried this out with the rectangle selector example, and it does not work at all in a way I'd expect. One edge barely rotated, while the other turns much more. Sometimes, the rectangle is a completely different aspect ratio. It becomes a parallelogram instead of a rotated rectangle.

That's because the rectangle is defined in axes coordinates, not figure coordinates. If you doax.set_aspect('equal') it should work as expected. I think it would take quite a bit more work to convertRectangleSelector to be drawn in figure coordinates instead of axes coordinates, but happy to give it a go if people think it's worth blocking this PR on.

Yeah I was just getting the point of wondering about that. Is it defined in axes coordinates or data coordinates? If data coordinates, I'm not clear how this is useful.

I've had a bit of a think about this, and would like to advocate for it to be included as is, with a note (or warning) in the docstring that it doesn't work as expected when the axes aspect ratio != 1. My reasoning is:

• It doesn't break or modify any existing behavior
• The motivating use case for this PR was selecting regions in astronomical images. These are almost always displayed with an aspect ratio of 1, where rotating selectors works.

Is it defined in axes coordinates or data coordinates?

It is defined in data coordinates, sorry for the confusion in my earlier comment.

@dstansby I understand your use case. OTOH I think its going to be confusing for everyone else when this rotation is not in figure/axes space... Is it hard to put in figure space?

@dstansby did you want a definitive answer on this? We could discuss on the call. I guess I'm -0.25 the way the PR is now. Can you implement outside of core?

Is it hard to put in figure space?

I suspect the answer is technically not hard, but it will take a reasonable amount of development time to disentangle everything to maintain backwards compatibility

Can you implement outside of core?

What do you mean by this? In another package?

I think the deciding question is: Would it be okay to release this as is, and then in the future change (ie. fix) the behavior for aspect!=1 without warning? If not, I'm happy to take a dive into moving the widgets from data to axes coordinates before merging this, but it might take a while.

I personally think this should be in figure space, but wouldn't block over it.

We should not inroduce broken rotations. That's rather confusing.

If you cannot make this work for the general case right now, I propose to only enable rotations on equal-aspect Axes. This should be easy to check viaself.ax.get_aspect() == 1.

If you cannot make this work for the general case right now, I propose to only enable rotations on equal-aspect Axes. This should be easy to check via self.ax.get_aspect() == 1.

Sounds good to me 👍

For a non-equal aspect axes, trying to rotate now does nothing and raises a warning. I've added an extra test to check this works as intended, and updated the docs.

Is it possible with reasonable effort to rotate about the center? The tilting over an edge is quite surprising and I don't know any other application doing this.

I just spent 5 minutes trying to work out if there was an easy way, and couldn't work it out, so I think changing this would take a reasonable amount of effort. This is mainly because aRectangle patch is defined by a corner coordinate and rotation, so it's non trivial (but possible) if one wants to set the rotation angle to be about the center, as a new rotation angle about the corner and corner coordinate has to be calculated for the rectangle at the same time.

I've rebased this, so should be good for re-review.

I just spent 5 minutes trying to work out if there was an easy way, and couldn't work it out, so I think changing this would take a reasonable amount of effort. This is mainly because aRectangle patch is defined by a corner coordinate and rotation, so it's non trivial (but possible) if one wants to set the rotation angle to be about the center, as a new rotation angle about the corner and corner coordinate has to be calculated for the rectangle at the same time.

Isn't the new location simply:x' = x + d - Md where x is the position vector of the rectangle corner, d is the connection vector from the corner to the center, and M is the rotation matrix?

Isn't the new location simply:x' = x + d - Md where x is the position vector of the rectangle corner, d is the connection vector from the corner to the center, and M is the rotation matrix?

Possibly? One also needs to work out the new rotation (about the patch corner). If someone wants to take a go at this feel free, I don't currently have the bandwidth to work out the maths or where it needs changing in the code.

One also needs to work out the new rotation (about the patch corner).

I think, you can just do the rotation and then shift the rectangle as proposed.

#20839 is an alternative PR to add selector rotation.

Done in#20839.