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

Updates anomaly docstrings #3537

Merged
merged 3 commits into from
Apr 12, 2017
Merged

Conversation

rhyolight
Copy link
Member

@rhyolight rhyolight commented Apr 11, 2017

WIP

fixes #3536

@rhyolight rhyolight requested review from marionleborgne and scottpurdy and removed request for marionleborgne and scottpurdy April 11, 2017 19:09
Copy link
Member

@marionleborgne marionleborgne left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Just one question, to make sure you did not delete something by mistake.

binaryAnomalyThreshold=None):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the doc string in the init?

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 have been moving all init docs into the class docs because it works better with Sphinx.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have good doc strings for __init__ functions. Usually the only info you need here is the parameter documentation. But I don't think we should move that to the class doc string just because of docs tooling.

Copy link
Member Author

Choose a reason for hiding this comment

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

@scottpurdy Can I add a docstring in each __init__ that points to the class definition? This seems like a common convention in python projects using Sphinx.

Copy link
Contributor

Choose a reason for hiding this comment

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

After looking into the Sphinx issue, I don't have a strong opinion. You might want to check with others since this is something all engineers will have to be aware of. But I'm fine with whatever route you want to go.

@rhyolight rhyolight merged commit 6eae371 into numenta:master Apr 12, 2017
@rhyolight rhyolight deleted the anomaly-docstrings branch April 12, 2017 13:47
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.

Update anomaly related docstrings
3 participants