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

Numpy docs hmc #2405

Merged
merged 5 commits into from
Jul 26, 2017
Merged

Numpy docs hmc #2405

merged 5 commits into from
Jul 26, 2017

Conversation

ColCarroll
Copy link
Member

As was foretold in #2403

@aseyboldt you might take particular offense to some of the docstrings -- would love any nitpicks.

@ColCarroll
Copy link
Member Author

(also, removing attribution to @jsalvatier is not meant as a slight -- I think the git history provides a record of credit, and this is interpreted by numpy as a malformed module docstring.)

@@ -3,3 +3,7 @@ testpaths = pymc3/tests

[coverage:run]
omit = *examples*

[pydocstyle]
add-ignore = D100,D104
Copy link
Member

Choose a reason for hiding this comment

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

what is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment -- D100 wants a docstring at the top of every file, and D104 wants a docstring in every __init__.py file

@@ -22,11 +18,17 @@ def unif(step_size, elow=.85, ehigh=1.15):


class HamiltonianMC(BaseHMC):
R"""A sampler for continuous variables based on Hamiltonian mechanics.

See NUTS sampler for automatically tuned stopping time.
Copy link
Member

Choose a reason for hiding this comment

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

stopping time and step size adaptation.

@@ -90,7 +91,8 @@ class NUTS(BaseHMC):
def __init__(self, vars=None, Emax=1000, target_accept=0.8,
gamma=0.05, k=0.75, t0=10, adapt_step_size=True,
max_treedepth=10, on_error='summary', **kwargs):
R"""
R"""Set up the No-U-Turn sampler.
Copy link
Member

Choose a reason for hiding this comment

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

The standard says that we shouldn't document the arguments in __init__ and in the class doc string.
https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#class-docstring

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like most of the class doc string is documenting the sampler_stats return object -- I hadn't seen that there is an Attribute section for class docstrings though, which is probably where the sampler stats should go...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, that is a funny thing to document in __init__ since it is an attribute it provides on the trace object. It makes sense to me to leave it where it is, and not adding an Attribute section.

@aseyboldt
Copy link
Member

Thanks for writing all those doc strings!

@ferrine
Copy link
Member

ferrine commented Jul 10, 2017

Don't we want new line after """?

Created on Mar 7, 2011

@author: johnsalvatier
'''
from ..arraystep import metrop_select, Competence
from .base_hmc import BaseHMC
from pymc3.vartypes import discrete_types
from pymc3.theanof import floatX
Copy link
Member

Choose a reason for hiding this comment

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

Numpy should be imported before local imports by pep8

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated this here -- it is actually a pylint check (wrong-import-order) that I can maybe try adding tomorrow, but that is a ton of violations!

@ColCarroll
Copy link
Member Author

Going to merge these once tests pass, since the rebasing is a pain and just making sure that the docs follow the functions around!

@twiecki
Copy link
Member

twiecki commented Jul 26, 2017

This is great @ColCarroll.

@twiecki twiecki merged commit 15b8595 into pymc-devs:master Jul 26, 2017
denadai2 pushed a commit to denadai2/pymc3 that referenced this pull request Aug 9, 2017
* Conform to numpy style docstrings in hmc submodule

* Accidentally messed with pylintrc

* Comments

* Rebase

* Rebase again
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.

5 participants