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

DOC: added examples to docstring of dirichlet class #9344

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

jeannefukumaru
Copy link

No description provided.

@jeannefukumaru
Copy link
Author

This is my first pull request to an open-source project. Any pointers on how to improve are much appreciated!

@ilayn
Copy link
Member

ilayn commented Oct 3, 2018

Hi @jeannefukumaru Thanks a lot for taking the time to prepare this. You are doing quite good so far. A few details to iron out:

  • In the docs, numpy is implicit to all examples, you can skip import numpy as np line
  • You might want to regroup similar items and decide on whether the explanations should be text or inline comment. Take ndimage.convolve as a random example. There are parts as explanations and parts as code (you can look at its source code how to do that). I think we can be a bit more economical here.
  • you can skip the assignments to d just to print. Hence instead of
    >>> d = dirichlet.pdf(quantiles, alpha)
    >>> d  
    0.2843831684937255
    you can directly type
    >>> dirichlet.pdf(quantiles, alpha)
    0.2843831684937255
  • I'll leave it up to your decision but either make it text or inline code comment for the initial 3 lines. Note that you can also use inline comments if it is short enough
    >>> dirichlet.rvs(alpha, size=2)  # get random samples from dirichlet
    array([[3.13387474e-02, 1.07631504e-01, 8.61029749e-01],
       [5.28822173e-05, 3.21909147e-01, 6.78037971e-01]])

@ilayn
Copy link
Member

ilayn commented Oct 3, 2018

You can also track what went wrong in the CI suite, for this case these are the documentation errors detected

https://travis-ci.org/scipy/scipy/jobs/436477657#L1369

@ilayn ilayn added the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Oct 8, 2018
@ilayn ilayn added this to the 1.2.0 milestone Oct 8, 2018
@ilayn
Copy link
Member

ilayn commented Oct 8, 2018

Thanks @jeannefukumaru, almost there, two three minor items remaining :)

that is because it needs empty lines also after the text blocks in the examples.

  • And also this line

once we specify the dirichlet distribution we can then calculate quantities of interest

is too long for 79 character limit.

  • The last one about PEP8-E261 which we don't actually track but since we are at it why not?

Otherwise it is good to go.

@jeannefukumaru
Copy link
Author

Thanks for the detailed feedback! It was very helpful. I've committed the edits you suggested

@ilayn
Copy link
Member

ilayn commented Oct 9, 2018

Due to the doctest requirements we have to add #may vary to places where the output can change. I've added those and removed the variable assignment to make things print.

scipy/stats/_multivariate.py Outdated Show resolved Hide resolved
@ev-br ev-br added the needs-work Items that are pending response from the author label Oct 12, 2018
@jeannefukumaru
Copy link
Author

jeannefukumaru commented Oct 24, 2018

Apologies for the delay and thanks for your patience. I've added the random_state keyword to dirichlet.rvs.

After looking at the details for why the CI builds are failing, it seems like python 2.7 and python 3.6 are two environments that are throwing the errors. Looks like the git checkout -qf FETCH_HEAD is giving a service error
screen shot 2018-10-24 at 3 48 28 pm

@ilayn
Copy link
Member

ilayn commented Oct 24, 2018

Travis Python 3.6 failure seems like a real one. The expected value for size = 2 is wrong. I get the same value as Travis CI reports

>>> dirichlet.rvs(alpha, size=1, random_state=1)
array([[0.00766178, 0.24670518, 0.74563305]])

>>> dirichlet.rvs(alpha, size=2, random_state=2)
array([[0.01639427, 0.1292273 , 0.85437844],
       [0.00156917, 0.19033695, 0.80809388]])

@rgommers rgommers removed this from the 1.2.0 milestone Oct 29, 2018
@ilayn
Copy link
Member

ilayn commented Oct 31, 2018

Since this is almost done, I've rebased it. If CI is happy it can go in.

@ilayn ilayn changed the title added examples to docstring of dirichlet class DOC: added examples to docstring of dirichlet class Oct 31, 2018
@ilayn ilayn removed the needs-work Items that are pending response from the author label Oct 31, 2018
@ilayn ilayn merged commit 4fc0b8c into scipy:master Nov 1, 2018
@ilayn ilayn added this to the 1.2.0 milestone Nov 1, 2018
@ilayn
Copy link
Member

ilayn commented Nov 1, 2018

Barely made it. Thanks @jeannefukumaru

@jeannefukumaru
Copy link
Author

Thanks so much for your patience @ilayn :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants