-
-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
Add spherical circle, range, polygon #306
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
# Licensed under a 3-clause BSD style license - see LICENSE.rst | ||
from astropy.coordinates import Angle, SkyCoord | ||
from regions.core.attributes import QuantityLength | ||
from ..core import SkyRegion, PixelRegion, PixCoord | ||
|
||
__all__ = ["RangePixelRegion", "RangeSphericalRegion"] | ||
|
||
inf = Angle(float("inf"), "deg") | ||
|
||
|
||
# TODO: needed? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed. Isn't it covered exactly by the existing planar rectangle as long as you choose the right parameterization? It might be thought of as a "convenience function". Maybe an additional constructor / factory function for the planar rectangle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there's a few small advantages to introducing
I'm still not sure if it's worth adding, but my feeling is that it might be the best way to go to add it? @gpdf - Thoughts? If no addition, what exactly whould be the alternative? |
||
class RangePixelRegion(PixelRegion): | ||
""" | ||
A range in pixel coordinates. | ||
|
||
See `RangeSphericalRegion`. | ||
""" | ||
|
||
|
||
class RangeSphericalRegion(SkyRegion): | ||
""" | ||
A range in longitude and latitude. | ||
|
||
Use case: http://www.ivoa.net/documents/SIA/20151223/REC-SIA-2.0-20151223.html#toc12 | ||
|
||
Parameters | ||
---------- | ||
lon_min, lon_max : `~astropy.coordinates.Angle` | ||
Range in longitude | ||
lat_min, lat_max : `~astropy.coordinates.Angle` | ||
Range in latitude | ||
frame : `~astropy.coordinates.BaseCoordinateFrame` | ||
Sky frame | ||
meta : `~regions.RegionMeta` object, optional | ||
A dictionary which stores the meta attributes of this region. | ||
visual : `~regions.RegionVisual` object, optional | ||
A dictionary which stores the visual meta attributes of this region. | ||
""" | ||
# TODO: should we use these properties, or SkyCoord for min / max, | ||
# or a center-based representation? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't use a center-based representation. That way lies the madness of ADQL's ill-fated BOX. A true range-based representation is fundamentally not relocatable in latitude - its shape is inherently different for different latitudes. A geometrically natural center-based representation needs to be fully relocatable - it should have the same shape anywhere on the sphere as long as its size parameters stay the same. (It's a coordinate-free geometrical concept.) But it's so likely to be confused with the coordinate-range region that people have a lot of trouble with it. BOX is being proposed for withdrawal from ADQL because it caused so much trouble of this kind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll leave the current representation then, as four angles and a separate frame attribute. |
||
_params = ('lon_min', 'lon_max', 'lat_min', 'lat_max') | ||
lon_min = QuantityLength('lon_min') | ||
lon_max = QuantityLength('lon_max') | ||
lat_min = QuantityLength('lat_min') | ||
lat_max = QuantityLength('lat_max') | ||
|
||
def __init__(self, lon_min=-inf, lon_max=inf, lat_min=-inf, lat_max=inf, frame="icrs", meta=None, visual=None): | ||
self.lon_min = lon_min | ||
self.lon_max = lon_max | ||
self.lat_min = lat_min | ||
self.lat_max = lat_max | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Python supports IEEE floating point -Inf and +Inf, so it should be possible, with care, to map these ranges exactly onto their IVOA SIAv2/SODA interpretation. It may require a bit of thinking to handle the edge case of a region that actually covers the entire sky. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'll add test cases. |
||
# TODO: Is there a helper function to make a frame without a | ||
self.frame = SkyCoord(0, 0, unit="deg", frame=frame).frame | ||
self.meta = meta or {} | ||
self.visual = visual or {} | ||
|
||
def to_pixel(self, wcs): | ||
sky_min = SkyCoord(self.lon_min, self.lat_min, frame=self.frame) | ||
sky_max = SkyCoord(self.lon_max, self.lat_max, frame=self.frame) | ||
|
||
pix_min = PixCoord.from_sky(sky_min, self.wcs) | ||
pix_max = PixCoord.from_sky(sky_max, self.wcs) | ||
return RangePixelRegion(pix_min.x, pix_max.x, pix_min.y, pix_max.y, self.meta, self.visual) | ||
|
||
def contains(self, skycoord, wcs=None): | ||
skycoord = skycoord.transform_to(self.frame) | ||
return ( | ||
(self.lon_min <= skycoord.data.lon) & | ||
(self.lon_max >= skycoord.data.lon) & | ||
(self.lat_min <= skycoord.data.lat) & | ||
(self.lat_max >= skycoord.data.lat) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Licensed under a 3-clause BSD style license - see LICENSE.rst | ||
from numpy.testing import assert_equal | ||
import astropy.units as u | ||
from astropy.coordinates import SkyCoord | ||
from ..range import RangeSphericalRegion | ||
|
||
# TODO: use test cases with finite & infinite values from here: | ||
# http://www.ivoa.net/documents/SIA/20151223/REC-SIA-2.0-20151223.html#toc12 | ||
|
||
def test_range_default(): | ||
region = RangeSphericalRegion() | ||
assert region.contains(SkyCoord("0d 0d")) | ||
|
||
def test_range_finite(): | ||
region = RangeSphericalRegion(12 * u.deg, 12.5 * u.deg, 34 * u.deg, 36.0 * u.deg) | ||
assert region.contains(SkyCoord("12d 34d")) | ||
assert not region.contains(SkyCoord("11d 34d")) | ||
assert not region.contains(SkyCoord("13d 34d")) | ||
assert not region.contains(SkyCoord("12d 33d")) | ||
assert not region.contains(SkyCoord("12d 37d")) | ||
|
||
mask = region.contains(SkyCoord([12, 13] * u.deg, [34, 34] * u.deg)) | ||
assert_equal(mask, [True, False]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment on #276 . I'd be happy to help review and/or write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gpdf - Thanks!
I'll add some docs in this PR, and then you can review.