Add ability to rotate ROIs and ROISubsetState

[dhomeier]

Description

WIP for implementing a rotation method on ROI shapes; starting with adding rotation angle as a property to the rectangular and elliptical ROIs.

Comments on implementation details:

• I've added the clockwise (do we have a specific preference for this? Perhaps change to anti-clockwise for consistency with matplotlib.patches?) rotation angle as self.theta, for now to RectangularROI, EllipticalROI, PolygonalROI, since that was already prepared in the ellipse class. This is understood as an "absolute" angle relative to the initialisation of the instance, so rotate_to() and rotate_by() allow changing this both as an absolute and additive modification.
• Rotation is always taken around the ROI centre, as rotating around another axis could in principle be effected by a combination of move_to and rotate_to. But an alternative axis point could still be added as an optional arg later. Again this differs partly from the matplotlib classes using the centre for Ellipse, but a corner anchor point for Rectangle.
• For RectangularROI and EllipticalROI the angle is needed as additional property to the size (width, height, semimajor axis etc.), so the latter always retain their _x and _y assignment even if no longer aligned with the original axes. For the rectangle that makes the meaning of xmin, xmax etc. rather confusing, but I could not think of a better way to initialise the ROI that is consistent with the current API (it would certainly be better to define it by width, height, centre now).
• As any rotated polygon is still a polygon in all generality, for PolygonalROI rotation is directly applied to the vertices. The angle is still stored so one might in principle restore the original orientation, but currently is not roundtripping, as the angle is not used in initialisation. I have made some attempts to add this to VertexROIBase.__init__(), but this would add a lot of overhead to the class and does not seem to be worth the effort (I think after adding or removing points there is also no way to restore the "original" polygon).

[dhomeier]

I've changed the sense of rotation to anticlockwise for consistency with matplotlib patches and projections, as well as astropy.modeling, erfa etc.

[dhomeier]

Marking this as ready for review, as the core functionality should be implemented for all classes where rotation can be sensibly applied.

• RangeROI might be up for discussion whether this should be generalised to, or supplemented by, a diagonal range.
• For Projected3dROI it is unclear to me if and how spatial rotation before projection to roi_2d might be applied.
• For the MplROIs all the selection methods only allow creating/modifying axes-aligned ROIs, so I think all one could do at this point is to support their initialisation with a rotated ROI. I don't see a method to move them with an existing ROI, so this does not seem terribly useful at this point – or are there already Mpl events that could be used for rotation?

[astrofrog]

I've added the clockwise (do we have a specific preference for this? Perhaps change to anti-clockwise for consistency with matplotlib.patches?) rotation angle as self.theta, for now to RectangularROI, EllipticalROI, PolygonalROI, since that was already prepared in the ellipse class. This is understood as an "absolute" angle relative to the initialisation of the instance, so rotate_to() and rotate_by() allow changing this both as an absolute and additive modification.

I would go with anti-clockwise, it's a pretty standard way of defining it.

Rotation is always taken around the ROI centre, as rotating around another axis could in principle be effected by a combination of move_to and rotate_to. But an alternative axis point could still be added as an optional arg later. Again this differs partly from the matplotlib classes using the centre for Ellipse, but a corner anchor point for Rectangle.

I would always use the center of the bounding box of the unrotated ROI, as that is consistent with what we might want to do UI wise (this is what I think most vector graphics programs will do).

For RectangularROI and EllipticalROI the angle is needed as additional property to the size (width, height, semimajor axis etc.), so the latter always retain their _x and _y assignment even if no longer aligned with the original axes. For the rectangle that makes the meaning of xmin, xmax etc. rather confusing, but I could not think of a better way to initialise the ROI that is consistent with the current API (it would certainly be better to define it by width, height, centre now).

What if we actually separated out the rotated and unrotated classes, so we'd have RotatedRectangularROI - and RectangularROI.rotated_by could return a RotatedRectangularROI for instance. This allows us to then be completely flexible with the API of the new classes without being constrained for 'historical reasons'.

As any rotated polygon is still a polygon in all generality, for PolygonalROI rotation is directly applied to the vertices. The angle is still stored so one might in principle restore the original orientation, but currently is not roundtripping, as the angle is not used in initialisation. I have made some attempts to add this to VertexROIBase.init(), but this would add a lot of overhead to the class and does not seem to be worth the effort (I think after adding or removing points there is also no way to restore the "original" polygon).

If we go with the above suggestion of splitting out the classes, one could have RotatedPolygonROI which stores the original vertices and rotation and computes the rotated vertices as needed on the fly - and we could then have a method that would allow one to return an 'evaluated' PolygonROI.

RangeROI might be up for discussion whether this should be generalised to, or supplemented by, a diagonal range.

Let's not worry about this - I think just adding rotation to the basic 2D shapes is enough.

For Projected3dROI it is unclear to me if and how spatial rotation before projection to roi_2d might be applied.

As above let's not worry about this.

For the MplROIs all the selection methods only allow creating/modifying axes-aligned ROIs, so I think all one could do at this point is to support their initialisation with a rotated ROI. I don't see a method to move them with an existing ROI, so this does not seem terribly useful at this point – or are there already Mpl events that could be used for rotation?

I think the right thing to do here will be to start investigating using the widgets in matplotlib.widgets to replace our own, and rotation is currently being added to them. But that's beyond the scope of this PR.

[astrofrog]

Could this logic be inside the rotation function.

[dhomeier]

Probably worth a shot, although after only calling rotation_matrix_2d as needed, the extra tests for exact multiples of right angles will still be needed in most places, to decide if the whole (much slower) process of pre-selecting and back-rotating points is to be initiated at all.

[dhomeier]

Moved from __init__ to __contains__, which is the caller for rotation_matrix_2d.

[astrofrog]

Sometimes when drawing a region one might end up with a different density of points around the edge, so I wonder if a better definition would be to use the center of the bounding box of the unrotated vertices - this will then be more meaningful once we are able to use the UI to rotate the polygon as this will likely be done by showing the bounding box of the polygon and dragging it around to rotate. This does mean that we should retain a reference to the unrotated vertices, which I think is sensible anyway.

[dhomeier]

The bounding box would at least save the worries if a starting node might be counted with double weight. But the bounding box has the disadvantage that it is not invariant under that rotation (unless we define "bounding box" to be aligned to x, y for the unrotated polygon only, and then rotate along with it). Having the centre change with rotations would make them non-commutative, and tumble the current concept of rotate_to and rotate_by.
Keeping the unrotated vertices is certainly good point either way, though this also leaves open what to do if vertices have been added or removed in between.

[dhomeier]

Having center depend on the density of vertices admittedly can be somewhat confusing, but it can be sensibly interpreted as a kind of centre of mass for the vertex collection...

[dhomeier]

Sometimes when drawing a region one might end up with a different density of points around the edge, so I wonder if a better definition would be to use the center of the bounding box of the unrotated vertices - this will then be more meaningful once we are able to use the UI to rotate the polygon as this will likely be done by showing the bounding box of the polygon and dragging it around to rotate. This does mean that we should retain a reference to the unrotated vertices, which I think is sensible anyway.

This is addressed by using self.centroid (see the "mock points" test).
Currently it is not keeping an explicit reference to the original vertices, just to the angle, which makes it possible in principle to restore them by rotating back to theta=0.
But I have still made the centre of rotation an optional input for now, since there may be legit reason to rotate around the mean position, bbox centre or even a corner or other point. This will produce confusing/inconsistent results when applying subsequent rotate_by operations around different centres though, so needs more discussion if this really is a good idea to allow.

[dhomeier]

Thanks for the review!

I would go with anti-clockwise, it's a pretty standard way of defining it.

Changed in a0a1e7f

Rotation is always taken around the ROI centre, as rotating around another axis could in principle be effected by a combination of move_to and rotate_to. But an alternative axis point could still be added as an optional arg later. Again this differs partly from the matplotlib classes using the centre for Ellipse, but a corner anchor point for Rectangle.

I would always use the center of the bounding box of the unrotated ROI, as that is consistent with what we might want to do UI wise (this is what I think most vector graphics programs will do).

Agreed, at least the matplotlib.widgets selectors seem to work that way, too. For polygons it's not quite that obvious (see inline comment).

For RectangularROI and EllipticalROI the angle is needed as additional property to the size (width, height, semimajor axis etc.), so the latter always retain their _x and _y assignment even if no longer aligned with the original axes. For the rectangle that makes the meaning of xmin, xmax etc. rather confusing, but I could not think of a better way to initialise the ROI that is consistent with the current API (it would certainly be better to define it by width, height, centre now).

What if we actually separated out the rotated and unrotated classes, so we'd have RotatedRectangularROI - and RectangularROI.rotated_by could return a RotatedRectangularROI for instance. This allows us to then be completely flexible with the API of the new classes without being constrained for 'historical reasons'.

Not sure if that's really worth creating 3-4 new classes; I've tried to implement the rotations in such a way that they always return the same class (there was a point in case for turning any rotated rectangle into a polygon, but the current implementation should have somewhat better performance).
Personally I would probably prefer to be able to just use the accustomed classes and specify an optional rotation parameter, and there is perhaps a point in having them round-trip properly (quite literally with 180-degree rotations). Although that does not work completely, e.g. a 90-deg rotated ellipse(a, b) is a different object from the ellipse(b, a), and a 180-deg rotated rectangle has its xmin, xmax swapped...
I was rather thinking of accepting xc, yx, width, height as additional optional parameters for RectangularROI analogously to EllipticalROI. Perhaps even to deprecate the xmin, xmax syntax at some point, although that could be problematic as they are certainly regularly used as positional arguments (which is however treacherous as well, since __init__ takes xmin, xmax, ymin, ymax whereas update_limits takes xmin, ymin, xmax, ymax).

For the MplROIs all the selection methods only allow creating/modifying axes-aligned ROIs, so I think all one could do at this point is to support their initialisation with a rotated ROI. I don't see a method to move them with an existing ROI, so this does not seem terribly useful at this point – or are there already Mpl events that could be used for rotation?

I think the right thing to do here will be to start investigating using the widgets in matplotlib.widgets to replace our own, and rotation is currently being added to them. But that's beyond the scope of this PR.

Yes, I think we should also wait until a decision between matplotlib/matplotlib#19864 and matplotlib/matplotlib#20839 is made.

[dhomeier]

I've moved the rotation matrix to .utils.geometry and removed all instances storing its results (even when calling contains() many times, the main work will likely go into the actual matrix multiplication with the points).
Leaving the class design as is for the moment.

[codecov]

Codecov Report

Merging #2235 (1298902) into main (31327f3) will decrease coverage by 0.00%.
The diff coverage is 89.43%.

@@            Coverage Diff             @@
##             main    #2235      +/-   ##
==========================================
- Coverage   88.14%   88.14%   -0.01%     
==========================================
  Files         247      247              
  Lines       23370    23472     +102     
==========================================
+ Hits        20600    20689      +89     
- Misses       2770     2783      +13     

Impacted Files	Coverage Δ
glue/utils/geometry.py	95.52% <80.00%> (-1.31%)	⬇️
glue/core/roi.py	91.49% <89.83%> (-0.43%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31327f3...1298902. Read the comment docs.

[astrofrog]

I think this looks good generally. I've noticed a couple of issues but I will leave it up to you whether to deal with it here or in a follow-up PR:

• If I update an ROI attached to a subset state with:

s.subset_state.roi.rotate_by(1)

then the viewers don't update immediately, instead I need to e.g. pan/zoom to force a refresh. This is actually a pre-existing issue because it happens with move_to. But ideally we should figure out whether we can somehow make sure a SubsetUpdateMessage is emitted for both translation and rotation.

• If I click and drag on an existing ROI, the selector (in yellow) is not aligned with the ROI (red) - but I think this happens on main already for me. However, if you could check that click and drag works for you with rotation enabled then that would be good.

Because neither of these are necessarily new issues, I am approving this - feel free to merge once you think it is good to go.

[dhomeier]

I am actually observing somewhat different behaviour between circular and rectangular ROIs; with the former the position during drag seems to jump to some 2.5 times the actual pointer position in pixel coordinates, but in the end the ROI appears in the correct position. For a rectangular selection it is flashing up, more or less as in your case, at larger and negative positions, but when the move is completed, seems to disappear altogether.
This happens identically both in main and this PR (and also independently from using data with a WCS or just a plain ndarray), so I guess needs a separate investigation how the event.*data are passed to the respective Mpl*ROI.update_selection().

CircularROI.mov

RectangularROI.mov