show sky coordinates in subset plugin when wcs linked

[cshanahan1]

This PR changes the behavior of the subset plugin to reflect link type (pixel or wcs). If wcs linked, subset attributes (e.g center, radius) will be displayed in the plugin in sky coordinates. If pixel linked, they will be displayed in pixels.

I added a subset_plugin.utils module to handle the translation between what is referred to in the plugin as 'subset_definition', a list of dictionaries each containing information about each subset attribute and regions/ROI/etc. This keeps most of the translations out of the actual plugin, and accessible by tests. I can think of some ways this could be improved in the future (defining a 'SubsetDefinition' class in the plugin, so that the structure of the dictionary doesn't have to be repeated so much), but in another PR.

Additionally, as referenced in the discussion below, we may want to remove the check if old==new upon update because this adds in another unnecessary pixel <> sky. I tried doing that by simply removing the check, and allowing it to change upon clicking 'update' even if the values didn't change, and I noticed that it wandered a bit while I did this presumably due to rounding and pixel>sky conversion internally. I do think this could be done smarter, but I'm not sure I want to change that here (I tried the comparison before conversion, and other errors came up, so I need to dig into this more).

Finally, the comment about rectangle corners not staying fixed upon changing one boundary is unfortunately how it's supposed to work. The rectangular subset xmin/xmax/ymin/ymax are the bounds before rotation, so the center can change. It is unintuitive but re defining it in terms of center/width/height will have to happen down the line.

[rosteen]

Displaying the RA/Dec of the subsets seems to work fine, however I'm not sure if updating the values in the plugin and then clicking update is doing the correct thing. I would have expected that if I updated one value (max RA in the video below) then at least one of the corners of the subset would remain in the same place as it started. However, it seems like the subset completely changes both shape and the location of all four corners when I update the max RA:

Screen.Recording.2023-11-15.at.2.35.44.PM.mov

Am I right that this is unexpected?

[cshanahan1]

@rosteen thanks for testing. to clarify, are you only seeing this issue for rectangle?

[rosteen]

Yes, the other shapes seem to behave in the way I would expect when updating their center or radius values.

[cshanahan1]

ok let me try to debug this. Theres a lot of room for error with that subset since i need to do many conversions between pixel and sky, and also between rectangles defined with xmin/xmax (for ROI) and height/width (for regions).

[cshanahan1]

I can't seem to prove that the behavior with the Rectangle (corner not staying in the same place when one min/max is changed) doesn't make sense. It doesn't seem right, but the conversion between pixel>sky, and region>ROI checks out:

1. Load the data, apply RectangularROI(xmin=2, xmax=5, ymin=2, ymax=5). Note the sky coordinates in the
   subset plugin because we are pixel linked:

2. run get_subsets to retrieve the subset we just created, which will return a pixel region and a sky region. Regions rectangle objects are defined with center/width/height, while the plugin works in min/max for ra/dec, so do this conversion (here i call a function that does this simple geometry). we can see that the bounds of the regions returned by get_subsets match what we see on the screen (pixel matches the ROI we drew, and sky matches what is in the plugin).

3. Now let's increment RA Max to 9.435 deg (well, it rounds to 9.434999 but close enough) leaving all the other bounds the same and click 'update':

4. And do the same procedure as we did with the initial subset - run get_subsets to get a PixelRegion and a SkyRegion
   from the new subset, and get the bounds of these regions and compare them to what we see on the screen/in the plugin:

The conversion checks out: Incrementing just RA Max to 9.435 and leaving RAMin/DecMin/DecMax the same results in a subset now bounded by pixels (1.7933849678375147, 5.476194759895096, 1.75001099816356, 4.7500110009957766). This is verified by the fact that the bounds of the SkyRegion returned by 'get_subsets' is what we see in the plugin, and what we adjusted it to (with only RA Max changed). The subset moves from a location bounded by (2.0, 5.0, 2.0, 5.0) to (1.7933849678375147, 5.476194759895096, 1.75001099816356, 4.7500110009957766). None of the corners are in the same place.

Where is the hole in this rationale? I can't figure it out!

[javerbukh]

I tried to create a composite subset (circle with a rectangular chunk removed) and see the following. Anyone else?

[pllim]

Is imviz.get_interactive_regions() behavior in scope for this or not?

[cshanahan1]

I'm not seeing the issue with composite subsets, I've been testing it out and it seems to work as intended, but I will keep looking into it

[cshanahan1]

I tested out the rectangle issue at theta=0 to take the angle out of it (rotation is done last), and I'm convinced that this is working as intended. Its unintuitive but this PR doesn't introduce that. I also tested 'recenter' and that continues to work with these changes (the UI is updated in sky coordinates when WCS linked as the subset moves)

[kecnry]

I know this isn't new code, but maybe we need a tech debt ticket to try to clean this up?

[kecnry]

did you try getting rid of these and just always updating the internal state? Does that cause problems? I just worry that the code-duplication and extra computations will have more cost than just pushing an extra update (with no changes).

[cshanahan1]

i haven't tried that yet, maybe this could be a follow up ticket since this already existed in the code before this PR? i'm fairly certain that removing this (and the check for if new matches old to avoid update) will just cause it to update every time, and is less costly than checking every time, but im worried about unintended consequences.

[kecnry]

hmm, ok, then is it possible to do the comparison in sky (store the orig sky in the dictionary instead of orig pixel and do the "change comparison" based on that)? It would just be nice to avoid duplicated code and minimize the number of conversions

[cshanahan1]

existing issue about composite subsets made with 'remove' mode. spacetelescope#2570.

[pllim]

Still 3 failures.

[pllim]

Suggested change

	mins_maxs = {'xmin': sky_region.center.ra.deg - ((sky_region.width).to(u.deg).value / 2.),
	'xmax': sky_region.center.ra.deg + ((sky_region.width).to(u.deg).value / 2.),
	'ymin': sky_region.center.dec.deg - ((sky_region.height).to(u.deg).value / 2.),
	'ymax': sky_region.center.dec.deg + ((sky_region.height).to(u.deg).value / 2.)}
	d_ra = sky_region.width.to_value(u.deg) * 0.5
	d_dec = sky_region.height.to_value(u.deg) * 0.5
	mins_maxs = {'xmin': sky_region.center.ra.deg - d_ra,
	'xmax': sky_region.center.ra.deg + d_ra,
	'ymin': sky_region.center.dec.deg - d_dec,
	'ymax': sky_region.center.dec.deg + d_dec}

[pllim]

Somehow this patch is loading an existing DS9 annulus region file incorrectly. You can try to run the code manually and debug outside the test session.

_____________ TestLoadRegions.test_regions_annulus_from_load_data ______________

self = <jdaviz.configs.imviz.tests.test_regions.TestLoadRegions object at 0x12bc3dfa0>

    def test_regions_annulus_from_load_data(self):
        # This file actually will load 2 annuli
        regfile = get_pkg_data_filename('data/ds9_annulus_01.reg')
>       self.imviz.load_data(regfile)

jdaviz/configs/imviz/tests/test_regions.py:154: 
...
E               ValueError: 'inner_radius' must be strictly positive

[pllim]

data_xy here is [None, None] with this patch. That is not supposed to happen. This test is set up to have two images, each with a bright pixel in one corner. This pixel ends up in different sky region because the two images have different WCSes. This tests is to ensure that the centering algorithm still works when linked by WCS.

_________ TestImvizSpatialSubsetCentroidWCSLinked.test_centroiding_wcs _________

self = <jdaviz.configs.imviz.tests.test_subset_centroid.TestImvizSpatialSubsetCentroidWCSLinked object at 0x12bc3cdc0>

    def test_centroiding_wcs(self):
        # FITS WCS and GWCS are rotated from each other.
        # Plain array always by pixel wrt FITS WCS.
        self.imviz.link_data(link_type='wcs')
    
        reg = CirclePixelRegion(PixCoord(2, 2), 3).to_sky(self.wcs_1)
        self.imviz.load_regions(reg)
    
        plg = self.imviz.plugins['Subset Tools']
        plg._obj.subset_selected = 'Subset 1'
        plg._obj.dataset_selected = 'fits_wcs[DATA]'
        plg._obj.vue_recenter_subset()
        data = self.imviz.app.data_collection[plg._obj.dataset_selected]
        # Pixel value is now w.r.t. fake WCS layer, not the selected data.
        for key in ("value", "orig"):
            x = plg._obj._get_value_from_subset_definition(0, "X Center", key)
            y = plg._obj._get_value_from_subset_definition(0, "Y Center", key)
            data_xy = self.viewer._get_real_xy(data, x, y)[:2]
>           assert_allclose(data_xy, -1)

jdaviz/configs/imviz/tests/test_subset_centroid.py:53: 

[pllim]

Similar problem as #19 (comment) . When linked by pixel, these two images have the bright pixel aligned on the same pixel, so both should return same value, but not None.

_______ TestImvizSpatialSubsetCentroidPixelLinked.test_centroiding_pixel _______

self = <jdaviz.configs.imviz.tests.test_subset_centroid.TestImvizSpatialSubsetCentroidPixelLinked object at 0x12bc3c160>

    def test_centroiding_pixel(self):
        reg = CirclePixelRegion(PixCoord(2, 2), 3)
        self.imviz.load_regions(reg)
    
        plg = self.imviz.plugins['Subset Tools']
        plg._obj.subset_selected = 'Subset 1'
    
        # Since they are linked by pixels, the bright corner pixel aligns
        # and nothing should change.
        for data_label in ('fits_wcs[DATA]', 'gwcs[DATA]'):
            plg._obj.dataset_selected = data_label
            plg._obj.set_center((2, 2), update=True)  # Move the Subset back first.
            plg._obj.vue_recenter_subset()
    
            # Calculate and move to centroid.
            for key in ("X Center", "Y Center"):
>               assert plg._obj._get_value_from_subset_definition(0, key, "value") == -1
E               assert None == -1

[cshanahan1]

@pllim i 'fixed' the test - the bug with loading regions is now the same as it is on main. the fix here was to not try to get a sky region when 'get_subsets' is run on init, but it should be safe to do this once the underlying issue is fixed.

ill link the follow up issue here.

[pllim]

BTW, I am fixing that test on main too. See spacetelescope#2589

Looks like you will need another approval from Brett to run the CI.

[bmorris3]

@cshanahan1 Still looks like one test related to this PR is failing: https://github.com/bmorris3/jdaviz/actions/runs/7107838227/job/19375803669?pr=19#step:5:695

[pllim]

Tsk tsk... cannot run the CI without spacetelescope#2595

[bmorris3]

Remaining failures will be solved by rebasing spacetelescope#2179 against main. Thanks @cshanahan1!