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

Adding support for rotating mpl selectors of Ellipse,RectanglePixelRegion #390

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

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Jul 29, 2021

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).
    ellipse-rotated
  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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@larrybradley
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

dhomeier commented Nov 30, 2021

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

RectanglePixelRegion-0
RectanglePixelRegion-30
RectanglePixelRegion-80

EllipsePixelRegion-0
EllipsePixelRegion-45

@dhomeier dhomeier force-pushed the rotated-mpl-selections branch 2 times, most recently from e41198d to f38f2cb Compare December 1, 2021 21:50
@dhomeier
Copy link
Contributor Author

dhomeier commented Dec 1, 2021

Seems azure no longer provides a py36 on macOS.

@keflavich
Copy link
Contributor

Could you rebase this?

Also, can we add support for circles?

@larrybradley
Copy link
Member

@keflavich You want to rotate a circle? 🤔

@dhomeier
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

larrybradley commented Feb 17, 2022

No CI tests except RTD?

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

@larrybradley
Copy link
Member

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

@dhomeier
Copy link
Contributor Author

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
Copy link
Contributor Author

One more upstream commit – strange...

@keflavich
Copy link
Contributor

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
Copy link
Member

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

@@ -59,6 +59,9 @@ New Features

- Added the DS9 'boxcircle' point symbol. [#387]

- Enable rotation of the ``as_mpl_selector`` widgets for rectangular
and ellipse regions with matplotlib versions supporting this. [#390]

Copy link
Member

Choose a reason for hiding this comment

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

This will need to be be moved to version 0.8.

MPL_VERSION = 10 * int(MPL_VERSION[0]) + int(MPL_VERSION[1])
except ImportError:
HAS_MATPLOTLIB = False
MPL_VER_STR = 'None'
Copy link
Member

@larrybradley larrybradley Sep 20, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@astrofrog astrofrog requested review from astrofrog and removed request for astrofrog October 24, 2023 10:05
Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

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

@dhomeier
Copy link
Contributor Author

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

RectanglePixelRegion-26833-0
RectanglePixelRegion-26833-1
RectanglePixelRegion-26833-2
RectanglePixelRegion-26833-3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants