allow override of wcs when computing sky region in roi_subset_state_to_region #96

cshanahan1 · 2023-10-06T03:30:45Z

In JDaviz (Cubeviz, specifically), sometimes subsets don't contain spatial wcs information in subset.xatt.coords, which is what is used for the pixel-to-sky conversion in roi_subset_state_to_region. To fix this issue, this PR adds the option to provide a different WCS for the conversion, which will override anything in .coords.

(See spacetelescope/jdaviz#2496)

glue_astronomy/translators/regions.py

pllim · 2023-10-10T18:25:25Z

glue_astronomy/translators/regions.py

+    to_sky: bool
+        Specifies if the range limits are for the x-axis or y-axis respectively.
+    override_wcs : WCS object
+        A world coordinate system (WCS) transformation that
+        supports the `astropy shared interface for WCS
+        <https://docs.astropy.org/en/stable/wcs/wcsapi.html>`_ (e.g.,
+        `astropy.wcs.WCS`, `gwcs.wcs.WCS`).


Suggested change

to_sky: bool

Specifies if the range limits are for the x-axis or y-axis respectively.

override_wcs : WCS object

A world coordinate system (WCS) transformation that

supports the `astropy shared interface for WCS

<https://docs.astropy.org/en/stable/wcs/wcsapi.html>`_ (e.g.,

`astropy.wcs.WCS`, `gwcs.wcs.WCS`).

to_sky: bool or `astropy.wcs.wcsapi.BaseHighLevelWCS`

If `False`, pixel region is returned; otherwise sky region.

If `True`, sky region will be computed using its parent's WCS.

If a WCS is given, then that would be used instead of its parent's WCS

(not recommended unless you know what you are doing).

Agreed as to giving a warning here in general, but then maybe one should also mention that parent may come without a valid WCS – after all this is the original use case, right?

And I have split feelings about packing this all into to_sky – while I was not really happy about the new kwarg's name, and do like the smaller footprint this way, it seems overloading that kwarg in a rather non-intuitive way. Just for your consideration.

i did something similar, how does that look?

pllim · 2023-10-10T18:26:22Z

glue_astronomy/translators/regions.py

@@ -93,7 +105,10 @@ def roi_subset_state_to_region(subset_state, to_sky=False):
        raise NotImplementedError(f"ROIs of type {roi.__class__.__name__} are not yet supported")

    if to_sky:


Suggested change

if to_sky:

glue_astronomy/translators/regions.py

dhomeier

See comments on @pllim's comments; I am undecided whether re-using the existing kwarg this way is the best option...

And it would be good to test this, e.g. add to the end of test_rectangular_roi something like

        self.data.coords = None
        with pytest.raises(**missing_wcs_error**):
            reg_sky = roi_subset_state_to_region(subset_state, to_sky=True)

        reg_sky = roi_subset_state_to_region(subset_state, to_sky=WCS_CELESTIAL)
        assert isinstance(reg_sky, RectangleSkyRegion)

Ideally this should perhaps even use two different WCS to make sure the user-provided is used; maybe one could instead pass in the second attempt a deepcopy of WCS_CELESTIAL modified to wcs.wcs.ctype = ['GLON-CAR', 'GLAT-CAR'] and then check reg_sky.wcs.ctype...

dhomeier · 2023-10-10T13:03:46Z

glue_astronomy/translators/regions.py

    return sky region from attached data WCS, otherwise it returns
    pixel region.
+


Suggested change

return sky region from attached data WCS, otherwise it returns

pixel region.

return the region transformed to sky coordinates per attached

data WCS or a custom WCS object passed as input in ``to_sky``,

otherwise it returns the region in pixel space.

modified to reflect the kwarg change

dhomeier · 2023-10-10T19:39:49Z

glue_astronomy/translators/regions.py

+    to_sky: bool
+        Specifies if the range limits are for the x-axis or y-axis respectively.
+    override_wcs : WCS object
+        A world coordinate system (WCS) transformation that
+        supports the `astropy shared interface for WCS
+        <https://docs.astropy.org/en/stable/wcs/wcsapi.html>`_ (e.g.,
+        `astropy.wcs.WCS`, `gwcs.wcs.WCS`).


Agreed as to giving a warning here in general, but then maybe one should also mention that parent may come without a valid WCS – after all this is the original use case, right?

And I have split feelings about packing this all into to_sky – while I was not really happy about the new kwarg's name, and do like the smaller footprint this way, it seems overloading that kwarg in a rather non-intuitive way. Just for your consideration.

dhomeier · 2023-10-10T20:15:47Z

glue_astronomy/translators/regions.py

+        if override_wcs is not None:
+            reg = reg.to_sky(override_wcs)
+        else:
+            reg = reg.to_sky(subset_state.xatt.parent.coords)


Suggested change

reg = reg.to_sky(subset_state.xatt.parent.coords)

reg = reg.to_sky(subset_state.xatt.parent.coords)

elif to_sky:

warnings.warn(f"Invalid input {to_sky} for ``to_sky``, "

"must be a valid WCS or set to `True`.",

UserWarning)

This is really for the change suggested by @pllim, but I cannot add suggestions on the suggestion...
But if we go that path, I think at that point (not isinstance(to_sky, BaseHighLevelWCS)) a sanity check is due, else it will appear to just silently ignore the to_sky input.

cshanahan1 · 2023-10-10T20:45:29Z

Looks like I pushed some commits i made before seeing the most recent review comments - I added tests (and moved the old one that tested to_sky) and implemented @pllim of overloading to_sky to allow a new WCS to override anything attached to the subset.

dhomeier · 2023-10-10T21:13:09Z

Thanks, tests look good! I still would just like to see the exception for an invalid or missing wcs in .coords (the original issue above) tested.

pllim · 2023-10-10T21:16:58Z

Ideally this should perhaps even use two different WCS to make sure the user-provided is used

Let's not do that. Ideally, subset state should always refer to its parent. But in Cubeviz, we have to hack around the fact that cube ingested into Cubeviz somehow contains .coords without the original spatial WCS info (I cannot find that issue for cross-linking right now). So ideally when this problem is fixed properly, no one will ever have to pass in to_sky as a WCS ever again.

glue_astronomy/translators/regions.py

pllim · 2023-10-10T21:21:38Z

glue_astronomy/translators/tests/test_regions.py

-        reg_sky = roi_subset_state_to_region(subset_state, to_sky=True)
-        assert isinstance(reg_sky, RectangleSkyRegion)


Why was this removed? This is a completely different test.

it was the only place that roi_subset_state_to_region with to_sky=True was being tested, it seemed to me like it was just thrown into an existing test to make sure it worked. i moved it to its own test (just using circle instead of rectangle. i could test every shape but that seems like overkill)

This one tests the rectangle. Your tests only have circles. I think it does not hurt to leave this in.

While I understand your point above, I don't see why extra testing here would hurt. This is not an intensive operation.

I agree the original test seemed thrown in a bit randomly, but if you really think Rectangle should be tested separately, perhaps do it properly indeed and pytest.parametrize over all supported ROI types (may indeed make sense, since the pixel_to_world transformations could be somewhat different under the hood).

Well, as a maintainer, Derek has the final say here, so I guess we can remove it. 🤷

Maybe parametrizeing the full test_to_sky over all ROI types with matching inputs and expected SkyRegions is a bit overkill indeed, so as a compromise you put that back in and add the same basic check to the tests for Polygonal, Elliptical and Point.

pllim · 2023-10-11T12:43:27Z

Actually.... I just realized that you do not really need this PR, @cshanahan1 . Since you only use this function to compute pixel region, and then convert to sky region internally in Jdaviz, given that you want to keep both pixel and sky.

😬

dhomeier · 2023-10-11T12:48:12Z

Let's not do that. Ideally, subset state should always refer to its parent. But in Cubeviz, we have to hack around the fact that cube ingested into Cubeviz somehow contains .coords without the original spatial WCS info

Thanks for the background – that explains why it all appears a bit hackish atm :D!
So you think no test for invalid input is needed either?

pllim · 2023-10-11T12:49:20Z

I actually don't think this PR is needed at all. I am going to revisit spacetelescope/jdaviz#2496 shortly.

dhomeier · 2023-10-11T12:55:57Z

Thanks; keep me posted! I think we may still want to keep some of the extra test coverage and doc updates.

cshanahan1 · 2023-10-11T13:20:31Z

I parameterized the tests which is also something we can keep even if we don't move forward with allowing 'to_sky' to be a WCS.

cshanahan1 · 2023-10-11T14:28:43Z

I'm going to open a PR on a new branch that just has the test + documentation changes we want to keep from this, and close this PR (but keep it separate in case we want to revisit this)

cshanahan1 · 2023-10-11T18:24:09Z

Closing this for now, doc + test changes we want to keep are in #97

allow override of wcs on subset to compute sky region

101d31f

cshanahan1 mentioned this pull request Oct 6, 2023

Add Sky Regions to get_subsets() spacetelescope/jdaviz#2496

Merged

9 tasks

pllim suggested changes Oct 10, 2023

View reviewed changes

remove keyword arg

3119950

dhomeier requested changes Oct 10, 2023

View reviewed changes

docstring

57f1797

fixes

9599cc7

pllim suggested changes Oct 10, 2023

View reviewed changes

parameterize test

ddadc28

cshanahan1 closed this Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow override of wcs when computing sky region in roi_subset_state_to_region #96

allow override of wcs when computing sky region in roi_subset_state_to_region #96

cshanahan1 commented Oct 6, 2023

pllim Oct 10, 2023

dhomeier Oct 10, 2023

cshanahan1 Oct 10, 2023

pllim Oct 10, 2023

dhomeier left a comment

dhomeier Oct 10, 2023 •

edited

Loading

dhomeier Oct 10, 2023

dhomeier Oct 10, 2023

cshanahan1 commented Oct 10, 2023

dhomeier commented Oct 10, 2023

pllim commented Oct 10, 2023

pllim Oct 10, 2023

cshanahan1 Oct 10, 2023 •

edited

Loading

pllim Oct 10, 2023

pllim Oct 10, 2023

dhomeier Oct 10, 2023

pllim Oct 10, 2023

dhomeier Oct 11, 2023

pllim commented Oct 11, 2023

dhomeier commented Oct 11, 2023

pllim commented Oct 11, 2023

dhomeier commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

		@@ -93,7 +105,10 @@ def roi_subset_state_to_region(subset_state, to_sky=False):
		raise NotImplementedError(f"ROIs of type {roi.__class__.__name__} are not yet supported")

		if to_sky:

		return sky region from attached data WCS, otherwise it returns
		pixel region.

-    return sky region from attached data WCS, otherwise it returns
-    pixel region.
+    return the region transformed to sky coordinates per attached
+    data WCS or a custom WCS object passed as input in ``to_sky``,
+    otherwise it returns the region in pixel space.

-            reg = reg.to_sky(subset_state.xatt.parent.coords)
+            reg = reg.to_sky(subset_state.xatt.parent.coords)
+        elif to_sky:
+            warnings.warn(f"Invalid input {to_sky} for ``to_sky``, "
+                          "must be a valid WCS or set to `True`.",
+                          UserWarning)

		reg_sky = roi_subset_state_to_region(subset_state, to_sky=True)
		assert isinstance(reg_sky, RectangleSkyRegion)

allow override of wcs when computing sky region in roi_subset_state_to_region #96

allow override of wcs when computing sky region in roi_subset_state_to_region #96

Conversation

cshanahan1 commented Oct 6, 2023

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 left a comment

Choose a reason for hiding this comment

dhomeier Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 commented Oct 10, 2023

dhomeier commented Oct 10, 2023

pllim commented Oct 10, 2023

Choose a reason for hiding this comment

cshanahan1 Oct 10, 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

pllim commented Oct 11, 2023

dhomeier commented Oct 11, 2023

pllim commented Oct 11, 2023

dhomeier commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

cshanahan1 commented Oct 11, 2023

dhomeier Oct 10, 2023 •

edited

Loading

cshanahan1 Oct 10, 2023 •

edited

Loading