Make ROI.move_to() behave consistently and expose it at subset_state level #2391

pllim · 2023-04-26T16:23:47Z

Make ROI.move_to() behave consistently such that all of them take absolute coords now, not some absolute and some delta. Also expose move_to machinery at subset_state level so we can move ROIs in composite subset all at once.

The main motivation is so I can have user click to recenter a subset that can be a simple shape with single ROI or an annulus that is actually a subset state with 2 attached ROIs (glue-viz/glue-astronomy#90).

I don't know why some move_to expects only delta but that kind of inconsistent behavior is more of a bug than feature from my perspective.

pllim · 2023-04-26T16:33:37Z

I cannot tell if Fatal Python error: Segmentation fault is related or not.

Since this fundamentally changed move_to for a couple of ROI classes, maybe you want to tell me what else I have to fix to make this work.

pllim · 2023-04-26T16:45:21Z

This error does look related. Any advice on how to fix the test? I am not familiar with that this scrubbing is supposed to do. Thanks!

____________________________ TestPolyMpl.test_scrub ____________________________

self = <glue.core.tests.test_roi.TestPolyMpl object at 0x7f091b8d1c90>
    def test_scrub(self):
        self.send_events()
        roi = self.scrub(roi=self.roi)
>       assert roi._roi.vx[0] == 6
E       assert -1.5 == 6
glue/core/tests/test_roi.py:1181: AssertionError

dhomeier · 2023-04-26T17:44:13Z

I don't know why some move_to expects only delta but that kind of inconsistent behavior is more of a bug than feature from my perspective.

I agree that's been bugging me before (may have been discussed in #2235, but I think there it was more about rotate_to/rotate_by). Behind this may be that at least CircularROI also has a set_center method, but that's been very inconsistently implemented (cf. #2208!). So I'd rather change set_center to self.move_to(x, y) now (and perhaps deprecate it at some point), and should it really be required, add a distinct move_by method.
The changed test results probably indicate that the Mpl*ROIs calling move_to within the scrubbing mechanism adapted to the old versions. I think this will need a bit more careful checking downstream to make sure this does not break anything.

For the test_scrub failure I think the correct fix would be to change MplPolygonalROI.update_selection to

        if self._scrubbing:
            self._roi.move_to(xval, yval)

pllim · 2023-04-26T18:14:58Z

Thanks for the pointers! I updated the code so that test result will not change.

dhomeier · 2023-04-26T21:31:49Z

glue/core/subset.py

+        """Move any underlying ROI to the new given center."""
+        self.state1.move_to(*args)
+        if self.state2:
+            self.state2.move_to(*args)


Thinking of your previous issue and comment on offset (non-concentric) AnnulusROIs, I am wondering if such CompositeSubsetStates could be constructed here. In that case the inner ROI would probably have to be moved as

Suggested change

self.state2.move_to(*args)

offset = self.state2.center() - self.state1.center()

self.state2.move_to(x + offset[0], y + offset[1], *args)

...

I don't understand your suggestion. I had tested this downstream and the annulus moved fine with my patch as-is. I am relying on the built-in state recursion.

Oh... I just saw "non-concentric". Is that a thing? I have not seen such things implemented in both regions and photutils.

Yes, not saying that this is presently implemented anywhere, but if it could be done in principle, better to allow for such a case.

But the subset state currently does not have a .center() and I am not 100% all ROIs have consistent .center() methods. This is not something I want to deal with in this patch. I'd rather we open up a follow up issue for non-concentric support.

Yes, CircularROI is lacking it, not sure of any others; but if it is not available for self.state2 generally, this can not be added here (I guess that means CompositeSubsetStates with different centres cannot be constructed ATM).

I mean, you could do composite subset state with whatever you want, and you correctly pointed out that move_to here won't work correctly for most of those except when they share the same center. I was hoping this won't be a problem for Jdaviz but now that I think about it... sigh, someone is going to draw something weird over there and encounter exactly this. 😭

So, why don't you and Tom hash out what is the plan with #2391 (comment) and then we can try to address that here? Thanks!

You might deprecate CircularROI.set_center here, but I'd also update all the tests to use move_to and then to complement it also implement center for everything (deprecating get_center where that is currently used).
The latter needs to be done for Circle, Ellipse and Polygon, which in turn needs some extra attention to return either centroid or mean. So I am fine with doing all of that in a separate PR; if you just have some tests to add for the move_to here, this should be good to merge.

dhomeier · 2023-04-26T21:36:10Z

Thanks, I did not trace the Polygon movement exactly, but [6, 7] looks rather like the correct position.

dhomeier · 2023-04-26T21:46:02Z

I have not found any direct uses of move_to() or set_center() downstream in any of the glueviz packages, so with MplROI taken care of, this might be safe.
I did notice that CircularROI of all things does not implement self.center, so would add that as a wrapper/replacement for get_center, and then also have set_center just wrap move_to. @astrofrog: do you think this would be a good moment for getting rid of set_center internally (again, it is only used in MplCircularROI and the tests, and nowhere downstream in glueviz afaics, but might be used in userland code, so I'd probably not remove it completely yet)?

astrofrog · 2023-04-27T10:56:26Z

Yes I think that would be fine - I doubt many people use this API so it is ok to update.

such that all of them take absolute coords now, not some absolute and some delta. Expose move_to machinery at subset_state level so we can move ROIs in composite subset all at once. Fix MplPolygonalROI.update_selection calculations. ROI classes now also have consistent center implementation. get_center and set_center methods are deprecated.

Co-authored-by: Derek Homeier <[email protected]>

when scrubbing

pllim · 2023-04-27T18:28:34Z

@dhomeier and @astrofrog , I think I addressed your comments. I tested the move_to with some weird composite subset and seems to work. It is reflected in the new tests I added.

dhomeier · 2023-04-27T20:50:28Z

glue/core/roi.py

@@ -865,7 +877,13 @@ def centroid(self):

        return np.dot(xs, dxy) * scl + x0, np.dot(ys, dxy) * scl + y0

-    def move_to(self, xdelta, ydelta):
+    def center(self):
+        return self.centroid()  # centroid is more robust than mean


Suggested change

return self.centroid() # centroid is more robust than mean

# centroid is more robust than mean, but undefined for degenerate (1D) polygons

if self.area() == 0:

return self.mean()

else:

return self.centroid()

I think that special case needs to be taken care of (although it seems not to be tested :( ), just as done in

glue/glue/core/roi.py

Lines 887 to 889 in d2adb68

# For linear (1D) "polygons" centroid is not defined.

if center is None:

if self.area() == 0:

– rotate_to can then be simplified to just use

center = self.center() if center is None else center

Good call. Done. Thanks!

Co-authored-by: Derek Homeier <[email protected]>

dhomeier

Huge thank you for taking care of all the refactoring and test updates!

pllim · 2023-05-02T16:54:47Z

Thanks for the reviews and merge!

astrofrog · 2023-05-04T12:30:09Z

I'm going to remove the Bug label because it's not really a bug, more of a clean-up, so I think I will bump the minor version as there is effectively an API change.

dhomeier · 2023-05-04T12:37:02Z

Agreed; should go under enhancement for the changelog then.

pllim mentioned this pull request Apr 26, 2023

POC: Circular annulus support in Imviz spacetelescope/jdaviz#2164

Closed

9 tasks

dhomeier added bug ROIs needs-more-tests labels Apr 26, 2023

dhomeier removed the needs-more-tests label Apr 26, 2023

dhomeier reviewed Apr 26, 2023

View reviewed changes

pllim and others added 4 commits April 27, 2023 12:10

Have state2 move relative to state1.

cd6dc2f

Co-authored-by: Derek Homeier <[email protected]>

Add tests

36e437e

Fix deprecation warnings in test run

2fcdc05

pllim force-pushed the gotta-move-it-move-it branch from f7dd8f7 to 2fcdc05 Compare April 27, 2023 18:23

Use center instead of centroid

5a35af9

when scrubbing

dhomeier reviewed Apr 27, 2023

View reviewed changes

Improve PolygonalROI.center() calc

44afab4

Co-authored-by: Derek Homeier <[email protected]>

dhomeier approved these changes Apr 28, 2023

View reviewed changes

dhomeier merged commit bd7a3a3 into glue-viz:main May 2, 2023

pllim deleted the gotta-move-it-move-it branch May 2, 2023 16:54

astrofrog removed the bug label May 4, 2023

dhomeier added the enhancement label May 4, 2023

pllim mentioned this pull request May 10, 2023

Enable editing of composite subsets in the subset plugin spacetelescope/jdaviz#2182

Merged

9 tasks

dhomeier mentioned this pull request May 11, 2023

Use CircularROI.center to avoid deprecation warnings with glue_core >= 1.10 glue-viz/glue-astronomy#91

Merged

This was referenced May 13, 2023

Add center and move_to methods to RangeSubsetState #2404

Merged

Use new API from glue-core spacetelescope/jdaviz#2204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ROI.move_to() behave consistently and expose it at subset_state level #2391

Make ROI.move_to() behave consistently and expose it at subset_state level #2391

pllim commented Apr 26, 2023 •

edited

Loading

pllim commented Apr 26, 2023

pllim commented Apr 26, 2023

dhomeier commented Apr 26, 2023

pllim commented Apr 26, 2023

dhomeier Apr 26, 2023 •

edited

Loading

pllim Apr 26, 2023

pllim Apr 26, 2023

dhomeier Apr 26, 2023

pllim Apr 26, 2023

dhomeier Apr 26, 2023

pllim Apr 26, 2023

dhomeier Apr 27, 2023

dhomeier commented Apr 26, 2023

dhomeier commented Apr 26, 2023

astrofrog commented Apr 27, 2023

pllim commented Apr 27, 2023

dhomeier Apr 27, 2023

pllim Apr 27, 2023

dhomeier left a comment

pllim commented May 2, 2023

astrofrog commented May 4, 2023

dhomeier commented May 4, 2023

	self.state2.move_to(*args)
	offset = self.state2.center() - self.state1.center()
	self.state2.move_to(x + offset[0], y + offset[1], *args)

-        return self.centroid()  # centroid is more robust than mean
+        # centroid is more robust than mean, but undefined for degenerate (1D) polygons
+        if self.area() == 0:
+            return self.mean()
+        else:
+            return self.centroid()

	# For linear (1D) "polygons" centroid is not defined.
	if center is None:
	if self.area() == 0:

Make ROI.move_to() behave consistently and expose it at subset_state level #2391

Make ROI.move_to() behave consistently and expose it at subset_state level #2391

Conversation

pllim commented Apr 26, 2023 • edited Loading

pllim commented Apr 26, 2023

pllim commented Apr 26, 2023

dhomeier commented Apr 26, 2023

pllim commented Apr 26, 2023

dhomeier Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhomeier commented Apr 26, 2023

dhomeier commented Apr 26, 2023

astrofrog commented Apr 27, 2023

pllim commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhomeier left a comment

Choose a reason for hiding this comment

pllim commented May 2, 2023

astrofrog commented May 4, 2023

dhomeier commented May 4, 2023

pllim commented Apr 26, 2023 •

edited

Loading

dhomeier Apr 26, 2023 •

edited

Loading