In December 2016 @joleroi @astrofrog and I had a Google hangout where we discussed astropy.regions
, the minutes are here:
https://docs.google.com/document/d/1u-WjsP4wLWx4g1-om7imstjngOmwpAJQ8EyVZxvhxV4/edit#heading=h.wf1hdss3eefb
I dropped the ball on implementing the changes discussed there, but I'd like to come back to that now. Actually, if this is non-controversial, I'd like to make the changes this week, before the regions 0.2 release, because it's not a lot of work, just removing a bunch of stuff instead of leaving dummy methods that return NotImplementedError
at the moment.
Before going ahead, I'd like to ask @astrofrog and @joleroi again, and also @keflavich and @larrybradley for input!
Basically we said that area
and contains
should be removed from the base sky region class:
http://astropy-regions.readthedocs.io/en/latest/api/regions.SkyRegion.html
The reason is that area
and contains
aren't well-defined concepts for all sky regions.
We argued about this for quite a while, but in the end I think there was agreement that it's not well-defined and thus doesn't belong in the base class.
E.g. how exactly would you implement area
and contains
for
http://astropy-regions.readthedocs.io/en/latest/api/regions.RectangleSkyRegion.html
so that it's valid for very large rectangles (say width = 100 deg)?
So we'd like to propose basically that we adopt the DS9 model of having regions mainly defined as pixel regions, and having sky regions mostly as convenience, but not with separate definitions of contains
methods. Instead contains
would be implemented in the following generic way on the base class for sky regions, and only be well-defined if the user has a WCS:
class SkyRegion(object):
def contains(self, sky_coord, wcs):
Reg = self.to_pixel(wcs)
Pix_coord = foo(sky_coord)
Return reg.contains(pix_coord)
In the future, we would add the special cases (circle & polygon) back where contains is well-defined in sky coordinates, either as separate classes or on the existing classes.
This brings us to the other major change we'd like to propose, which is to get rid of the different modes in the pixel to sky transformation:
This was never implemented, it was just an idea, there will be no functionality lost.
So for now, we'd remove the modes from the base class, only leaving the "mode" how DS9 transforms back and forth, and we'd remove all the checks and NotImplementedError from the derived classes.
Note that regions
will remain experimental, and based on user and dev feedback, we can always do it differently in the end. So basically -- if there's agreement on this I would make the change tomorrow, before the 0.2 release. And if it's controversial, I would just make the release as-is, and we take our time to discuss here.