Add spherical circle, range, polygon #306
Conversation
| inf = Angle(float("inf"), "deg") | ||
|
|
||
|
|
||
| # TODO: needed? |
There was a problem hiding this comment.
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.
I think there's a few small advantages to introducingRangePixelRegion (instead of using RectanglePixelRegion):
- Clear code, symmetry, being able to round-trip when doing
to_pixelandfrom_pixelisntead of mixing over to the rectangle classes - I'm not sure how the +- inf case would be handled. I don't think one can handle the
min=-inf, max=42case, there's no center that will result in a rectangle extending that range? - It's not a lot of work (30 lines of code?)
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?
Should RangeSphericalRegion not offer to_pixel at all, or should it go to RectanglePixelRegion?
| 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.
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.
OK, I'll leave the current representation then, as four angles and a separate frame attribute.
| 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.
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.
| class CircleSphericalRegion(CircleSkyRegion): | ||
| """Spherical circle sky region. | ||
|
|
||
| TODO: document |
|
Unfortunately I don't have time for this any more, closing it now. See #276 (comment). |
I'd like to add spherical circle, range and polygon functionality to `astropy-regions.
This PR follows the thought in #276 (comment) to add those as extra classes.
I'm not sure if that is the way to go, thus the PR in the early draft state.
For
RangeSphericalRegionspecifically I've left a few questions as# TODO- please make suggestions.@larrybradley @astrofrog @eteq @gpdf - Thoughts?