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.

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

Implement compound regions #26

Merged
merged 2 commits into from
May 4, 2016
Merged

Implement compound regions #26

merged 2 commits into from
May 4, 2016

Conversation

joleroi
Copy link
Contributor

@joleroi joleroi commented May 3, 2016

  • contains

I don't know how to deal with area, make_patch etc.

@joleroi
Copy link
Contributor Author

joleroi commented May 3, 2016

This also changes the API as discussed in #25

@joleroi
Copy link
Contributor Author

joleroi commented May 4, 2016

I would merge this and add further functionality in future PRs. Comments?

@@ -29,16 +29,21 @@ class CompoundSkyRegion(SkyRegion):
Represents the logical combination of two regions in sky coordinates.
"""

def __init__(self, region1, operator, region2):
def __init__(self, region1, region2, operator):
Copy link
Member

Choose a reason for hiding this comment

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

The reason I had operator in the center is because it makes it clearer how to deal with non-symmetric cases (e.g. region 1 without region 2)

Copy link
Contributor Author

@joleroi joleroi May 4, 2016

Choose a reason for hiding this comment

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

There was a bug (e.g. https://github.com/astropy/regions/blob/master/regions/core/core.py#L67) . It was just easier to change this line instead of all the others .. Do you need the operator in the middle?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see - I personally think it would make it more readable, but up to you :)

Copy link
Contributor Author

@joleroi joleroi May 4, 2016

Choose a reason for hiding this comment

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

I'm more the RPN guy, you know 😉

Copy link
Member

Choose a reason for hiding this comment

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

😆

@astrofrog
Copy link
Member

astrofrog commented May 4, 2016

@joleroi - that sounds fine. I have just one comment above at the moment. I think we will need some additional operators (for example the equivalent of - for sets) but we can add that later.

@astrofrog
Copy link
Member

Indeed, area and make_patch/as_patch will be tricky for compound regions...

@joleroi joleroi merged commit 7b8aab6 into astropy:master May 4, 2016
@joleroi joleroi deleted the compound branch May 4, 2016 12:49
@cdeil cdeil added this to the 0.1 milestone Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants