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

Added scatter variable to realize_lcs so that we can turn off scatter … #106

Merged
merged 3 commits into from
Aug 25, 2015

Conversation

rbiswas4
Copy link
Member

…in realized light curves

@@ -128,6 +128,11 @@ def realize_lcs(observations, model, params, thresh=None,
If True, only observations with times between
``model.mintime()`` and ``model.maxtime()`` are included in
result table for each SN. Default is False.
scatter : bool, optional, defaults to True
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: "Default is True" should go at the end of the description, not on this line. See above parameter for example.

@rbiswas4
Copy link
Member Author

Fixed docstring by moving the default description to the end. Is that just the convention in this code, or is it something closer to pep8 and I should be trying to follow that even more generally?

Could not figure out why is the travis ci check failing ?

@kbarbary
Copy link
Member

Change looks good. The specific format of docstrings are not specified by PEP8. Different projects use different conventions, but astropy and sncosmo use the numpydoc convention, which is detailed here: https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt (search for "default"). The location of the default description is a very minor thing, but its better to just be consistent with the rest of the code.

The travis failure is caused by a PEP8 error (line too long). run pep8 sncosmo --exclude=version.py locally to reproduce. You can also see this on travis by clicking on "Details" in the box above, then clicking on the failing build, then reading the log.

@rbiswas4
Copy link
Member Author

The location of the default description is a very minor thing, but its better to just be consistent with the rest of the code.

Completely agree on the consistency issue.

kbarbary added a commit that referenced this pull request Aug 25, 2015
Added scatter variable to realize_lcs so that we can turn off scatter
@kbarbary kbarbary merged commit 16abea4 into sncosmo:master Aug 25, 2015
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

2 participants