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

[pllim]

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 (https://github.com/glue-viz/glue-astronomy/pull/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]

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]

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]

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]

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

[dhomeier]

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

[dhomeier]

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]

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

[pllim]

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

[pllim]

Thanks for the reviews and merge!

[astrofrog]

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]

Agreed; should go under enhancement for the changelog then.