Adding support for rotating mpl selectors of Ellipse,RectanglePixelRegion

[dhomeier]

This is part 1 of a follow-up to #317 to support as_mpl_selector for rotated regions and rotating them, if the matplotlib version allows this (currently this needs the changes from matplotlib/matplotlib#19864 for the added functionality). Points for discussion:

1. This implementation makes use of the private properties _rotate and _rect_properties to determine the geometry of the rotated shapes; I do not see a public method to conveniently access the rotation angle, and getting the correct position and width/height from extents or others is convoluted at minimum.
2. The plotted circumference of a rotated EllipsePixelRegion becomes offset from the bounding box, while the mask area seems positioned correctly. My understanding is that this is still drawn incorrectly from mpl's widgets.py, possibly because _draw_shape is also not using the correctly transformed centre, but I haven't found a way to correct for that yet (if the error really is on the mpl side).
   
3. Tests should probably be complemented by (at least) one for rotation operation, either added to test_as_mpl_selector or as a new one? I guess this would also need to use the ax.figure.canvas.callbacks.process for a
   KeyEvent('key_press_event', ax.figure.canvas, 'r') – pointers how to use this are welcome.

[dhomeier]

And of course existing and new tests would need a setup with matplotlib/matplotlib#19864 to test the actual functionality, but at least I could get started with •3 on my local installation.

[larrybradley]

Yes, and if we are going to rely on mpl private attributes, then we'll definitely need to add a CI test against mpl dev.

[dhomeier]

Maybe it's worth waiting if angle, width and height are exposed better as the mpl PR advances.

[larrybradley]

Yes, that sounds good. I just realized that matplotlib/matplotlib#19864 is still a draft PR. Perhaps it's worth contacting the author about exposing those attributes and also the potential bug with the bounding box offset.

[dhomeier]

Yes, also just noticed it had been marked ready for review and then changed back to draft.
Just wanted to confirm the drawing of the offset ellipse is not done somewhere on our side / in the example adopted from #317, but it doesn't look like that.

[dhomeier]

Now mpl has a competing PR for enabling rotation in matplotlib/matplotlib#20839; haven't got to compare the two in detail or checked if this PR will work with the other approach, but maybe it's best to wait a while which one comes out on top.

[dhomeier]

Updated to the API from matplotlib/matplotlib#20839 now; the extents of the mask on a rotated selector have slightly changed now at the % level (see test outcomes for the sync case); might warrant further inspection, but for interactive use is probably not problematic.
Also on the plus side the EllipseSelector and its bounding box are now properly aligned (includes a fix to _axes.py)!

[dhomeier]

Seems azure no longer provides a py36 on macOS.

[keflavich]

Could you rebase this?

Also, can we add support for circles?

[larrybradley]

@keflavich You want to rotate a circle? 🤔

[dhomeier]

@keflavich You want to rotate a circle? 🤔

If it's rotated around a bbox corner – but that will still require support on the matplotlib first.
I thought until then one might kind of export it to EllipsePixelRegion.

Or is it about adding a mpl_selector to it in the first place – that would be more in the line of #406?

[keflavich]

ha, no, sorry, I don't want to rotate a circle =)

but it does deserve to have its own selector. Sorry, that is a different issue.

[dhomeier]

No CI tests except RTD?
I tested locally against Matplotlib: 3.6.0.dev1657+gc2f39dddb5 ; would require to add something like
devdeps: git+https://github.com/matplotlib/matplotlib.git#egg=matplotlib

[larrybradley]

No CI tests except RTD?

That's odd. Do you have .github/workflows/ci_tests.yml in your branch?

[larrybradley]

(you may need to pull the latest version of main and then rebase)

[dhomeier]

I just checked I have the 581e81a version of https://github.com/dhomeier/regions/blob/3bff348e1596620e968438abeb04ed40ec7e9a0f/.github/workflows/ci_tests.yml (literally rebased an hour ago or so...)
I was also looking for differences to PRs that did run, but seems there has been none since switching to GH Actions.

[dhomeier]

One more upstream commit – strange...

[keflavich]

I've finally gotten to play with this a little more, and the rotation selector works in that it rotates the region... but it doesn't follow the cursor.

Example:

from spectral_cube import SpectralCube
from regions import PixCoord, EllipsePixelRegion
import pylab as pl
cube = SpectralCube.read('http://www.astropy.org/astropy-data/l1448/l1448_13co.fits')
fig = pl.figure()
ax = fig.gca()
pl.imshow(cube.max(axis=0).value)
ellipse = EllipsePixelRegion(center=PixCoord(x=66, y=33), width=21, height=10,
                             angle=-23*u.deg, visual={'color': 'yellow'})
selector = ellipse.as_mpl_selector(ax)

when I press r and start dragging around, the ellipse rotates, but not in any way obviously correlated with how I move my cursor.

Also, there's no visual indicator about whether I'm in rotate or stretch mode; it would be nice to switch the corners to angled arrows or something that indicates we're rotating. That might be an upstream request.

If these issues are entirely upstream, though, I'd recommend merging this and noting that there are some UI issues,

[larrybradley]

IIRC, this feature requires matplotlib 3.6 (specifically this PR: matplotlib/matplotlib#20839), which is not released yet.

[larrybradley]

This will need to be be moved to version 0.8.

[larrybradley]

Is it possible to streamline the matplotilb version checking? Something like what astropy uses for numpy version checks (see https://github.com/astropy/astropy/blob/main/astropy/utils/compat/numpycompat.py)?

from astropy.utils import minversion

MPL_LT_3_6 = not minversion(matplotlib, '3.6')

and then use it the code with:

if MPL_LT_3_6:
  ...

[dhomeier]

Thanks for reviewing; will look into it.
Want to check first if this can be made to work with matplotlib/matplotlib#21945, which I've just reopened as matplotlib/matplotlib#26833 – unfortunately switching to display coordinates seems to break not just rotation but our existing resize and move operations; getting errors about self.height = ymax - ymin in _update_from_mpl_selector being set to negative values...

[larrybradley]

Thanks. Let me know when this is ready for a (final?) review.

[astrofrog]

@dhomeier - what is needed at this point (here and on the matplotlib side) so that we can move ahead with this?

[dhomeier]

The blocking problem is that with the change to display coordinates as described in #390 (comment) any interaction (not even rotation, just normal move/resize) on y coordinates runs into wildly off vertical coordinates, quickly raising exceptions

Traceback (most recent call last):
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/matplotlib/cbook.py", line 298, in process
    func(*args, **kwargs)
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/matplotlib/widgets.py", line 2367, in release
    self._release(event)
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/matplotlib/widgets.py", line 3455, in _release
    self.onselect(self._eventpress, self._eventrelease)
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/regions/shapes/rectangle.py", line 218, in _update_from_mpl_selector
    self.height = ymax - ymin
    ^^^^^^^^^^^
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/regions/core/attributes.py", line 41, in __set__
    self._validate(value)
  File "/Users/derek/opt/mambaforge/envs/regions/lib/python3.11/site-packages/regions/core/attributes.py", line 90, in _validate
    raise ValueError(f'{self.name!r} must be a strictly positive '
ValueError: 'height' must be a strictly positive scalar

Even in the initial display the outline (unfortunately not drawn in the exported plot) does not match the handles but extends higher up (where the mask has moved in the next picture, after just clicking inside the region).