Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
101d31f
allow override of wcs on subset to compute sky region
cshanahan1 Oct 6, 2023
3119950
remove keyword arg
cshanahan1 Oct 10, 2023
57f1797
docstring
cshanahan1 Oct 10, 2023
9599cc7
fixes
cshanahan1 Oct 10, 2023
ddadc28
parameterize test
cshanahan1 Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions glue_astronomy/translators/regions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from glue import __version__ as glue_version

from astropy import units as u
from astropy.wcs.wcsapi import BaseHighLevelWCS
from packaging.version import Version
from regions import (RectanglePixelRegion, PolygonPixelRegion, CirclePixelRegion,
PointPixelRegion, PixCoord, EllipsePixelRegion,
Expand Down Expand Up @@ -56,9 +57,25 @@ def range_to_rect(data, ori, low, high):
def roi_subset_state_to_region(subset_state, to_sky=False):
"""Translate the given ``RoiSubsetState`` containing ROI
that is compatible with 2D spatial regions to proper
``regions`` shape. If ``to_sky=True`` is given, it will
return sky region from attached data WCS, otherwise it returns
pixel region.
``regions`` shape.

If ``to_sky`` is False, it will return the region in pixel coordinates.
If ``to_sky=True``, it will return the region transformed to sky
coordinates, per attached data WCS. Alternatively, ``to_sky`` can be a WCS
object, which will override any WCS on the input subset state data and the
region will be returned in pixel coordinates.

Parameters
----------
subset_state : `~glue.core.subset.SubsetState`
ROI subset state.
to_sky: bool or WCS object
If True, return region in celestial coordinates from attached data WCS.
Optionally, if a 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`) - is provided, then this will
override the WCS attached to the subset data.
"""
roi = subset_state.roi

Expand Down Expand Up @@ -92,9 +109,12 @@ def roi_subset_state_to_region(subset_state, to_sky=False):
else:
raise NotImplementedError(f"ROIs of type {roi.__class__.__name__} are not yet supported")

if to_sky:
if to_sky is True:
if subset_state.xatt.parent.coords is None:
raise ValueError('Subset parent does not have a WCS.')
reg = reg.to_sky(subset_state.xatt.parent.coords)

elif isinstance(to_sky, BaseHighLevelWCS):
reg = reg.to_sky(to_sky)
return reg


Expand Down
87 changes: 63 additions & 24 deletions glue_astronomy/translators/tests/test_regions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import pytest
import numpy as np
from astropy import units as u
from astropy.wcs import WCS
from astropy.tests.helper import assert_quantity_allclose
from numpy.testing import assert_allclose, assert_array_equal, assert_equal
from packaging.version import Version

from regions import (RectanglePixelRegion, RectangleSkyRegion,
PolygonPixelRegion, CirclePixelRegion,
EllipsePixelRegion, PointPixelRegion, CompoundPixelRegion,
CircleAnnulusPixelRegion, PixCoord)
from regions import (RectanglePixelRegion, RectangleSkyRegion, PolygonPixelRegion,
CirclePixelRegion, CircleSkyRegion,
EllipsePixelRegion, PointPixelRegion, PointSkyRegion,
CompoundPixelRegion, CircleAnnulusPixelRegion, PixCoord)

from glue.core import Data, DataCollection
from glue.core.roi import (RectangularROI, PolygonalROI, CircularROI, EllipticalROI,
Expand All @@ -32,25 +33,37 @@ def setup_method(self, method):
self.data.coords = WCS_CELESTIAL
self.dc = DataCollection([self.data])

def test_rectangular_roi(self):
@pytest.fixture
def setup_rois(self):
# define different ROI shapes that are used in several tests.

subset_state = RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
RectangularROI(1, 3.5, -0.2, 3.3))
rois = {'CircularROI': RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
CircularROI(1, 3.5, 0.75)),
'RectangularROI': RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
RectangularROI(0, 2, 0, 7)),
'PointROI': RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
PointROI(1, 3.5))
}
return rois

def test_rectangular_roi(self, setup_rois):

subset_state = setup_rois['RectangularROI']

self.dc.new_subset_group(subset_state=subset_state, label='rectangular')

reg = self.data.get_selection_definition(format='astropy-regions')

assert isinstance(reg, RectanglePixelRegion)

assert_allclose(reg.center.x, 2.25)
assert_allclose(reg.center.y, 1.55)
assert_allclose(reg.width, 2.5)
assert_allclose(reg.height, 3.5)
assert_allclose(reg.center.x, 1)
assert_allclose(reg.center.y, 3.5)
assert_allclose(reg.width, 2)
assert_allclose(reg.height, 7)

reg_sky = roi_subset_state_to_region(subset_state, to_sky=True)
assert isinstance(reg_sky, RectangleSkyRegion)
Comment on lines -52 to -53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cshanahan1 cshanahan1 Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def test_polygonal_roi(self):

Expand All @@ -70,11 +83,9 @@ def test_polygonal_roi(self):
assert_array_equal(reg.vertices.x, xv)
assert_array_equal(reg.vertices.y, yv)

def test_circular_roi(self):
def test_circular_roi(self, setup_rois):

subset_state = RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
CircularROI(1, 3.5, 0.75))
subset_state = setup_rois['CircularROI']

self.dc.new_subset_group(subset_state=subset_state, label='circular')

Expand Down Expand Up @@ -111,20 +122,18 @@ def test_ellipse_roi(self, theta):
assert_equal(reg.height, 10)
assert_quantity_allclose(reg.angle, theta * u.radian)

def test_point_roi(self):
def test_point_roi(self, setup_rois):

subset_state = RoiSubsetState(self.data.pixel_component_ids[1],
self.data.pixel_component_ids[0],
PointROI(2.64, 5.4))
subset_state = setup_rois['PointROI']

self.dc.new_subset_group(subset_state=subset_state, label='point')

reg = self.data.get_selection_definition(format='astropy-regions')

assert isinstance(reg, PointPixelRegion)

assert_equal(reg.center.x, 2.64)
assert_equal(reg.center.y, 5.4)
assert_equal(reg.center.x, 1)
assert_equal(reg.center.y, 3.5)

def test_xregion_roi(self):

Expand Down Expand Up @@ -488,3 +497,33 @@ def test_unsupported(self):
match='Subset states of type InequalitySubsetState are not supported'):
self.data.get_selection_definition(format='astropy-regions',
subset_id='Flux-based selection')

@pytest.mark.parametrize("subset_state, output_pixel_shape, output_sky_shape",
[('CircularROI', CirclePixelRegion, CircleSkyRegion),
('RectangularROI', RectanglePixelRegion, RectangleSkyRegion),
('PointROI', PointPixelRegion, PointSkyRegion)])
def test_roi_subset_state_to_region(self, subset_state, output_pixel_shape,
output_sky_shape, setup_rois):
# test returning pixel/sky regions with `roi_subset_state_to_region`

subset_state = setup_rois[subset_state]

# test that only pixel region is returned when to_sky is False
reg_pixel = roi_subset_state_to_region(subset_state, to_sky=False)
assert isinstance(reg_pixel, output_pixel_shape)

reg_sky = roi_subset_state_to_region(subset_state, to_sky=True)
assert isinstance(reg_sky, output_sky_shape)
assert_allclose(reg_sky.center.ra.deg, 1.99918828)
assert_allclose(reg_sky.center.dec.deg, 4.48805907)

# test overriding WCS
override_wcs = WCS(naxis=2)
override_wcs.wcs.ctype = ['RA---TAN', 'DEC--TAN']
override_wcs.wcs.crval = [0, -90]

reg_sky = roi_subset_state_to_region(subset_state, to_sky=override_wcs)
assert isinstance(reg_sky, output_sky_shape)
assert_allclose(reg_sky.center.ra.deg, 23.96248897)
assert_allclose(reg_sky.center.dec.deg, -85.08764318)