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: clarify expect() return val when moment is inf/nan #9267

Merged
merged 4 commits into from
Oct 14, 2018

Conversation

rlucas7
Copy link
Member

@rlucas7 rlucas7 commented Sep 14, 2018

Closes #9259

@rlucas7 rlucas7 force-pushed the scipy_issue_9259 branch 2 times, most recently from d84b2b8 to 278a753 Compare September 15, 2018 13:30
@rlucas7
Copy link
Member Author

rlucas7 commented Sep 15, 2018

Also, I noticed I had put numeric integration in the discrete method definition. While not incorrect in the Lebesgue measure sense (just switching to counting measure) this is likely unclear to the typical doc reader so in the latest commit (force pushed to remote branch) I changed to numeric summation

@josef-pkt
Copy link
Member

josef-pkt commented Sep 15, 2018

To help in understanding the effect of bounds an example in the docstring with integrating constant function might be useful.

i.e. expect of lambda x: 1 is just cdf(ub) - cdf(lb)
if conditional=True, then expect of lambda x: 1 is just 1

@rlucas7
Copy link
Member Author

rlucas7 commented Sep 17, 2018

@chrisb83 , @josef-pkt, I'm new here should I close the PR if all the comments are addressed?

Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

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

Hi, the text is fine, i added a few formatting details. Please don't close the PR, it will be merged by one of the members once approved.

scipy/stats/_distn_infrastructure.py Outdated Show resolved Hide resolved
scipy/stats/_distn_infrastructure.py Outdated Show resolved Hide resolved
scipy/stats/_distn_infrastructure.py Outdated Show resolved Hide resolved
@chrisb83 chrisb83 added the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Sep 22, 2018
@rlucas7
Copy link
Member Author

rlucas7 commented Sep 22, 2018

@chrisb83, Thanks for all your time (and keen eyes!) so far, super helpful. All check are passing. Do you see any other issues in the commit?

Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good now. Only one minor thing: It would be nice to have the examples in a section "Examples" but this can also be done in a later PR.

@rlucas7
Copy link
Member Author

rlucas7 commented Oct 2, 2018

@chrisb83, Thanks! I added in 2 lines separation with "Examples" header in lines 2394 + 2395. Unclear if we want the "for example" examples from the previous ¶ in the new examples section. The doc seems to read better with them in the previous ¶ so I left them in that ¶.

@chrisb83
Copy link
Member

chrisb83 commented Oct 3, 2018

I think it is fine, normally the examples are written in a way that the code is run and the user sees the results, see e.g. https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.jarque_bera.html
this can also be changed in a follow up PR, in any case, there is more Information for the user now than before.

let's wait a few more days if there is further feedback, otherwise I will merge it

@rlucas7
Copy link
Member Author

rlucas7 commented Oct 3, 2018

let's wait a few more days if there is further feedback, otherwise I will merge it

will do.

@rlucas7
Copy link
Member Author

rlucas7 commented Oct 13, 2018

@chrisb83, it's been a few days and no one has responded. Should I ping the others contributors on the PR?


Examples
--------
To understand the effect of the bounds of integration consider the
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the last minute review but can we make this as code examples

To understand the effect of the bounds of integration consider

    >>> from scipy.stats import expon
    >>> expon(1).expect(lambda x: 1, lb=0.0, ub=2.0)
    0.6321205588285578

This is close to 

    >>> expon(1).cdf(2.0) - expon(1).cdf(0.0)
    0.6321205588285577

If ``conditional=True``

    >>> expon(1).expect(lambda x: 1, lb=0.0, ub=2.0, conditional=True)
    1.0000000000000002

The slight deviation from 1. is due to numerical integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ilayn, thanks. I added the examples docstring similar to how you have it here. and pushed. The builds all seem to pass except the Travis build which only fails on python 3.5 and the tests that are failing are totally unrelated (in signals sub-module)-or at least appear to be-to the changes. All other python versions in the travis build pass the checks. Maybe these are some flaky tests?

@ilayn
Copy link
Member

ilayn commented Oct 14, 2018

The fails are unrelated (I'll submit a PR).

So thanks @rlucas7 @chrisb83 merging.

@ilayn ilayn merged commit 4256945 into scipy:master Oct 14, 2018
@ilayn ilayn added this to the 1.2.0 milestone Oct 14, 2018
@rlucas7
Copy link
Member Author

rlucas7 commented Oct 14, 2018

@ilayn + @chrisb83 Thanks for shepherding me through this process! 👍

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.

5 participants