Rotate image with glue Affine transform #1551
Conversation
|
I think @astrofrog is already on the weird translation behaviour; one oddity I noticed when applying the Affine2D transform directly on the viewer state in the Imviz example: Don't know if that tells something about how the transform behaves with different WCS... |
pllim
force-pushed
the
canvas-rotation-again
branch
from
5d423da
to
bdfca0b
Compare
As you correctly surmised during the SME meeting, the actually relevant difference is the (non-)square aspect ratio – turns out the 'x', 'y' parameters to mpl's So providing the centre coordinates the other way around the rotation appears to remain well in place: As a note on the last Todo, I have run the above example (3 NIRSpec datasets of ~16000 * 7800 pixels) on my old 16 GB i5, which takes 1 or 2 seconds on the rotation – short comparable to the initial Since the pointer is not shown in the screenshot, I also note that it was in the upper right, the life info showing the viewport position with the data for those pixel coordinates, i.e. not following the rotation – the actual pointer position is outside the image area here. |
Hmm, so coordinates info panel is wrong when canvas is rotated? That is a big problem. How do we tell glue-jupyter to give us the mouse event data that is consistent with what is shown? |
|
p.s. Whoa, which colormap is that? 🤩 |
|
Just manually picked colours for the three filters. 😜 |
What do you mean? You should be able to with this: jdaviz/jdaviz/configs/imviz/helper.py Line 189 in 207f7a3
|
Exactly that; perfect, thanks! (Did not scroll down far enough in the notebook...) |
Any hints where/how those data might be returned from glue-jupyter? @astrofrog – any thoughts if the wrapper in bqplot |
The event data we get comes from glue-jupyter + bqplot somewhere. jdaviz/jdaviz/configs/imviz/plugins/viewers.py Lines 52 to 53 in feadcb9 I think it is the data info for the reference data tied to a particular viewer, which has given me WCS transformation headaches when one isn't viewing the reference data but I have since worked around that in Imviz. |
Might be worth trying to apply the (inverse?) jdaviz/jdaviz/configs/imviz/plugins/viewers.py Lines 108 to 109 in feadcb9 but I think this would indee, if possible, be better handled on the bqplot side, although I don't see where data['domain'] might be populated inside glue_jupyter.
|
I'd rather glue-jupyter or bqplot handles this for us. Otherwise, Jdaviz will have to dig through all the event handling code and figure out when to inverse the transform... |
pllim
force-pushed
the
canvas-rotation-again
branch
from
5b54002
to
e62f3e9
Compare
There was a problem hiding this comment.
I've updated my two branches/PRs so this will now require some changes here mentioned below.
Things that don't yet work as far as I can tell:
- If you change the rotation, the 'grey' active selection shape doesn't follow. I think we should clear it if the rotation changes
- We should probably provide a method in glue-core on the state class which given cursor coordinates can provide the actual array index into the data since these are now no longer the same - we can then use that to fix the mouse over coordinates here
@dhomeier - would you be able to look into both of these?
pllim
force-pushed
the
canvas-rotation-again
branch
from
e62f3e9
to
fdab918
Compare
|
|
||
| # Rotate selected viewer canvas. | ||
| # TODO: Translation still a bit broken if zoomed in. | ||
| viewer.state.rotation = math.radians(self._theta) |
There was a problem hiding this comment.
Works but pretty sure it is clockwise again... Is this expected?
Also, the center translation is still a bit off when I zoom in first and then rotate on a rectangular image. If I zoom in on a star, I would expect rotation around the star. But the star moves quite dramatically when I rotate.
There was a problem hiding this comment.
Apparently applying the transform directly on x, y in the pretransform.__call__() seems to have it operate the other way around again, but that is easily fixed, even if I cannot follow the full chain of events.
I think for finding the right centre of rotation setting xy from (x_min + x_max) / 2 rather than shape[0] / 2 etc. is still the better option (x_min, y_min may be nonzero even if not zoomed in); at least that that keeps the viewport centred and rotated around the original zoom region when zooming in first and then rotating. Unfortunately with the same modification, zooming in after rotation is completely off – apparently this is setting the limits from the unrotated frame, I recon this is failing due to the data values not being returned from the transformed pixel coordinates just as with the mouseover.
I would expect that once the latter problem is solved, the zoom will also operate on the correct subset, so the alternative above still looks a step forward.
There was a problem hiding this comment.
Derek, all this will be fixed upstream, yes?
There was a problem hiding this comment.
Not sure which of the "all" is fixable upstream. I have a patch for the sense of direction, and keeping the centre of rotation in the current viewport centre, but the mouseover stats, active selection area and zoom-after-rotation remain of.
- We should probably provide a method in glue-core on the state class which given cursor coordinates can provide the actual array index into the data since these are now no longer the same - we can then use that to fix the mouse over coordinates here
I think I'll work on that next, but it sounds like this will still require some adaption from Imviz to use those viewer.state methods.
There was a problem hiding this comment.
If it helps, the Imviz mouseover logic is here:
Out of scope now but if you are curious, Cubeviz version is here:
They both send info to the same plugin here:
There was a problem hiding this comment.
Yes; looked at the above, but can I find any more info on what type of object the data is that is passed to on_mouse_or_key_event? I've been struggling to find out what its 'domain' item is or where it is set.
There was a problem hiding this comment.
That I am not sure... Could be glue-jupyter , bqplot-image-gl , bqplot ... 🤷
Hopefully @maartenbreddels or @astrofrog can tell us.
|
|
||
| # TODO: Zoom box in Compass still broken, how to fix? If not, we need to disable it. | ||
| # Update Compass plugin. | ||
| viewer.on_limits_change() |
There was a problem hiding this comment.
How do I get viewer limits that is consistent with the rotation, so I can show a sane zoom box in Compass plugin when image is rotated?
There was a problem hiding this comment.
Are you going to show the image rotated in the compass plugin as well? Or are you going to show it in the original/native orientation and want to show the box rotated? (since it matters for the limits)
There was a problem hiding this comment.
I was thinking it is easier in Compass if we do not rotate the image but only rotate the zoom box. The Compass display real estate is already pretty small. To rotate the image in Compass would make the image display even smaller when rotated.
Also even if not rotated, images can have wild orientations after you link them by WCS.
Codecov Report
@@ Coverage Diff @@
## main #1551 +/- ##
==========================================
- Coverage 86.12% 85.60% -0.53%
==========================================
Files 94 96 +2
Lines 9318 9237 -81
==========================================
- Hits 8025 7907 -118
- Misses 1293 1330 +37
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I've pushed a change to glue-viz/glue#2310 which will preserve the field of view when changing the rotation, so if you are centered on a star and rotate the canvas then the star will still be at the center. This is the way that DS9 does it and I think it is the most intuitive. For the compass, I think that we should also use the DS9 approach - rotate the image and keep the box in the same orientation as the main image view. The easiest way to achieve this is that in the compass imshow() call, we should specify transform= and give it the same affine transform we use in glue - for now if you want to avoid having to re-construct it yourself you can access the glue one with |
|
I've pushed more changes to both the glue-core and glue-jupyter PR so be sure to update these. I've also opened pllim#6 which shows how to fix the compass plugin to correctly show image rotation. One thing I've noticed is that if I modify the rotation using the plugin, the view seems to update a few times which is due to the part of the code that deals with the aspect ratio calculation. This shouldn't be happening though, we should be delaying calls to the aspect ratio so that it just happens once. So something isn't working quite right there. I see the issue in glue-jupyter too but not in Qt glue so the issue must be in glue-jupyter. I will investigate! |
|
@astrofrog / @dhomeier -- Should I hold off until more work upstream is in? |
|
I think it would be valuable to already try it as is |
|
I've put two suggestions here and in glue-viz/glue#2310 to fix the case where a pretransform is not yet defined; otherwise this should be good to work with. |
|
I can't test this right now because my jdaviz installation isn't working properly, but I think you should be able to fix the coordinate overlay by doing: # If needed, transform x/y to the actual rotated reference data
# frame of reference
if self.state.rotation != 0:
x, y = self.state._affine_pretransform(x, y)just before the Altough if you think the results don't make sense you might need |
|
Also a note that we are going to have to do something about the markers as currently those won't be rotated - will have a think. |
[ci skip]
Co-authored-by: Derek Homeier <[email protected]>
Looks like angle is clockwise again, not counter-clockwise.
Co-authored-by: Derek Homeier <[email protected]>
Co-authored-by: Derek Homeier <[email protected]>
pllim
force-pushed
the
canvas-rotation-again
branch
from
d0dba04
to
5b29340
Compare
|
I've had a long think about this and I think as promising as this approach seemed initially, it is now getting a lot more complicated when it comes to dealing properly with markers, and I am also seeing some issues with multiple redraws that are tricky to solve. This is going to require either a lot of special casing in the glue-core and glupyter code base, or duplication of a lot of code (but modified) from glue in jdaviz, neither of which are desirable. I think that we should consider one of the following approaches:
In the second case, and as in the present PR, one of the limitations is that if the user is ever exposed to pixel coordinates (for example through mouse-over or editing the limits of the plot) the pixel coordinates would be those of the rotated image. |
Is that bad?
I am interested to hear if @maartenbreddels has any opinions on the options. |
|
Maybe a good topic for the next SME call? I don't have a good overview of the different approaches and cons and pros. |
|
@astrofrog , did you make any progress on this? If there is some new proof-of-concept ready for us to play with, please let us know. Thanks! |
|
No, unfortunately it will be week or so before I can get back to this. |
|
Superseded by #1904 |
Description
This pull request is to use glue Affine transform to rotate Imviz image.
Depends on:
Fixes #1466
TODO
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?