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

LLS: Some edits for the LLS GUI #103

Merged
merged 3 commits into from
Dec 3, 2015
Merged

LLS: Some edits for the LLS GUI #103

merged 3 commits into from
Dec 3, 2015

Conversation

profxj
Copy link
Contributor

@profxj profxj commented Dec 3, 2015

A new method or two
and a test

class GenericAbsSystem(AbsSystem):
"""Class for Generic Absorption Line System
"""
def __init__(self, radec, zabs, vlim, **kwargs):
if vlim is None:
if vlim is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate statements.

@profxj
Copy link
Contributor Author

profxj commented Dec 3, 2015

Fixed.

Anything else?

class GenericAbsSystem(AbsSystem):
"""Class for Generic Absorption Line System
"""
def __init__(self, radec, zabs, vlim, **kwargs):
if vlim is None:
vlim = [-500.,500.]*u.km/u.s
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still thinking whether this is a good idea or not. If you do something like this for vlim, then for the sake of symmetry I would also allow radec and zabs to have default values, which may be useful in some cases where you do not care about those quantities. On the other hand, for the cases you do care, having default values may cause some trouble...

Copy link
Contributor

Choose a reason for hiding this comment

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

How about having them as optional parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, fair point.

I'll go back to what we had.

ntejos added a commit that referenced this pull request Dec 3, 2015
 LLS: Some edits for the LLS GUI
Looks good to me then. Merging.
@ntejos ntejos merged commit b3f850d into linetools:master Dec 3, 2015
@coveralls
Copy link

coveralls commented Oct 16, 2017

Coverage Status

Changes Unknown when pulling f8772d8 on profxj:LLS into ** on linetools:master**.

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