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

Add Line{Circle|Sky}Region #129

Merged
merged 2 commits into from
May 10, 2017
Merged

Add Line{Circle|Sky}Region #129

merged 2 commits into from
May 10, 2017

Conversation

joleroi
Copy link
Contributor

@joleroi joleroi commented May 9, 2017

One step towards fixing #45

@joleroi joleroi self-assigned this May 9, 2017
@joleroi joleroi requested a review from keflavich May 9, 2017 13:36
"""

def __init__(self, start, end, meta=None, visual=None):
self.start = PixCoord._validate(start, name='start', expected='scalar')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a better way to instantiate PixCoords? It seems weird to use private methods (_validate) here

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Few small things, but ready to merge.

"""
def __init__(self, start, end, meta=None, visual=None):
# TODO: test that start, end is a 0D SkyCoord
self.start = start
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we merge without having the is_coord check? Or just add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This (and also the PixCoord._validate issue) are unrelated to this PR - the same issue is present for all regions at the moment. So I would go for merging this and fixing the input validation separately.

@property
def area(self):
"""Region area (`~astropy.units.Quantity`)"""
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

0*u.sr?

raise NotImplementedError()

else:
raise ValueError('mode should be one of local/affine/full')
Copy link
Contributor

Choose a reason for hiding this comment

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

is mode irrelevant here?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is not! All modes do the same.

@joleroi joleroi mentioned this pull request May 9, 2017
@joleroi
Copy link
Contributor Author

joleroi commented May 9, 2017

@keflavich , others : Currently the line __init__ takes start and end. For the parser it would be easier if this was actually a len 2 list of SkyCoords (like a Polygone with 2 vertices). Opinions?

@joleroi joleroi merged commit 65486da into astropy:master May 10, 2017
@joleroi joleroi deleted the line branch May 10, 2017 14:17
@cdeil cdeil modified the milestones: 0.2, 0.3 Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants