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

Simplify region base classes #107

Closed
cdeil opened this Issue Feb 14, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@cdeil
Member

cdeil commented Feb 14, 2017

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.

@cdeil cdeil added the question label Feb 14, 2017

@cdeil cdeil self-assigned this Feb 14, 2017

@cdeil cdeil added this to the 0.2 milestone Feb 14, 2017

@keflavich

This comment has been minimized.

Show comment
Hide comment
@keflavich

keflavich Feb 14, 2017

Collaborator

I have no objections to your proposed changes. I haven't considered the implications too deeply, but the arguments you laid out seem reasonable. Most use cases I can think of involving 'contains' are indeed in pixel space or expressible in a cartesian coordinate representation (e.g., I want to know all objects in a box in Galactic coordinates) and can be special cased.

Collaborator

keflavich commented Feb 14, 2017

I have no objections to your proposed changes. I haven't considered the implications too deeply, but the arguments you laid out seem reasonable. Most use cases I can think of involving 'contains' are indeed in pixel space or expressible in a cartesian coordinate representation (e.g., I want to know all objects in a box in Galactic coordinates) and can be special cased.

@larrybradley

This comment has been minimized.

Show comment
Hide comment
@larrybradley

larrybradley Feb 22, 2017

Member

@cdeil This sounds fine to me.

Member

larrybradley commented Feb 22, 2017

@cdeil This sounds fine to me.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 7, 2017

Member

@cdeil - are you or @joleroi planning on working on the extensive changes we discussed on the hangout originally? Or is more discussion needed first?

Member

astrofrog commented May 7, 2017

@cdeil - are you or @joleroi planning on working on the extensive changes we discussed on the hangout originally? Or is more discussion needed first?

@cdeil cdeil removed their assignment May 7, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil May 7, 2017

Member

Sorry I didn't find time to do this yet.
I've just now unassigned myself in case someone is interested to work on this issue this week.
I think it's pretty clear and there seems to be agreement, so probably time for one or several small pull requests.
If no-one works on this, I plan to come back to this in a few weeks.

Member

cdeil commented May 7, 2017

Sorry I didn't find time to do this yet.
I've just now unassigned myself in case someone is interested to work on this issue this week.
I think it's pretty clear and there seems to be agreement, so probably time for one or several small pull requests.
If no-one works on this, I plan to come back to this in a few weeks.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 11, 2017

Member

I'm going to sprint on this at #pyastro17 now

Member

astrofrog commented May 11, 2017

I'm going to sprint on this at #pyastro17 now

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog

astrofrog May 16, 2017

Member

This should be done in #132

Member

astrofrog commented May 16, 2017

This should be done in #132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment