Skip to content

Correcting that p should be in [0,1] for a variety of discrete distns. #12962

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 18, 2020
Merged

Correcting that p should be in [0,1] for a variety of discrete distns. #12962

merged 3 commits into from
Oct 18, 2020

Conversation

WillTirone
Copy link
Contributor

DOC: adding that p in [0,1] for several discrete distributions in stats/_discrete_distns.py for issue #12952

Reference issue

Closes: #12952

What does this implement/fix?

Since p is a probability, and probabilities must be 0 <= p <= 1, I've added this for the relevant discrete distributions.

Additional information

  1. Discrete distribution testing worked before and after changes were added
  2. Please let me know if there is a way to "test render" the text in the docstring to ensure that it will display properly online.
  3. I'm still a beginner at working with the scipy repo so please let me know if anything can be improved related to the code / git / etc etc.

Thanks!

@WarrenWeckesser WarrenWeckesser added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats labels Oct 18, 2020
@WarrenWeckesser WarrenWeckesser added this to the 1.6.0 milestone Oct 18, 2020
WarrenWeckesser
WarrenWeckesser requested changes Oct 18, 2020
View reviewed changes
Copy link
Member

@WarrenWeckesser WarrenWeckesser left a comment

Choose a reason for hiding this comment

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

Thanks @WillTirone. I requested a few changes inline.

scipy/stats/_discrete_distns.py Outdated
f(k) = \binom{k+n-1}{n-1} p^n (1-p)^k
for :math:`k \ge 0`.
for :math:`k \ge 0`, :math:'0 \leq p \leq 1'
Copy link
Member

Choose a reason for hiding this comment

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

Strict inequality on the low end:

Suggested change
for :math:`k \ge 0`, :math:'0 \leq p \leq 1'
for :math:`k \ge 0`, :math:`0 < p \leq 1`

Copy link
Contributor Author

@WillTirone WillTirone Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If this requires a strict inequality, should the _argcheck also include the strict inequality? for nbinom_gen it is as follows:

 def _argcheck(self, n, p):
        return (n > 0) & (p >= 0) & (p <= 1)

I guess if so that would be part of a different commit to address that.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, that should be (p > 0) (likewise for geom). If you are interested, you can make those changes in this PR, or we can do that in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll just add that to this one - that seems simple enough to include.

scipy/stats/_discrete_distns.py Outdated
f(k) = (1-p)^{k-1} p
for :math:`k \ge 1`.
for :math:`k \ge 1`, :math:'0 \leq p \leq 1'
Copy link
Member

Choose a reason for hiding this comment

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

Strict inequality on the low end:

Suggested change
for :math:`k \ge 1`, :math:'0 \leq p \leq 1'
for :math:`k \ge 1`, :math:`0 < p \leq 1`

Copy link
Contributor Author

@WillTirone WillTirone Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This also has the same _argcheck as negative binomial:

    def _argcheck(self, p):
        return (p <= 1) & (p >= 0)

@WarrenWeckesser
Copy link
Member

  1. Please let me know if there is a way to "test render" the text in the docstring to ensure that it will display properly online.

Instructions for building the docs are in doc/README.md. If you have any trouble with that, feel free to ask here or on the mailing list for help.

Once you've submitted a PR, you can view the rendered docs online by clicking on the Details link of the CircleCI test labeled ci/circleci: build_docs artifact.

@WillTirone
Copy link
Contributor Author

  1. Please let me know if there is a way to "test render" the text in the docstring to ensure that it will display properly online.

Instructions for building the docs are in doc/README.md. If you have any trouble with that, feel free to ask here or on the mailing list for help.

Once you've submitted a PR, you can view the rendered docs online by clicking on the Details link of the CircleCI test labeled ci/circleci: build_docs artifact.

Awesome, thank you. I'll check that out.

@WarrenWeckesser
Copy link
Member

Thanks @WillTirone, the updates look good. I'll merge once the last tests finish.

@WarrenWeckesser WarrenWeckesser merged commit db8d870 into scipy:master Oct 18, 2020
@WillTirone
Copy link
Contributor Author

Thanks for your help @WarrenWeckesser !

@josef-pkt
Copy link
Member

why is there an asymmetry now in whether boundary points are included?

p=0 and p=1 should in most cases be treated the same.

(n > 0) & (p >= 0) & (p <= 1)

@josef-pkt
Copy link
Member

PRs with changes in code should be clearly labeled as such

if p=0 is a bug, then it should be a bug fix PR

e.g #12916 (comment) adding ravel as doc fix

@WarrenWeckesser WarrenWeckesser added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label Oct 18, 2020
@WarrenWeckesser
Copy link
Member

p=0 and p=1 should in most cases be treated the same.

I don't see why that should be true in general. Whether or not the end point is a well-defined probability distribution depends on the properties of the distribution. For nbinom and geom, p = 0 does not result in a well-defined probability distribution, since in both cases, the PMF P(k) is 0 for any k. For p = 1, however, they are both well-defined (and trivial).

@WarrenWeckesser
Copy link
Member

if p=0 is a bug, then it should be a bug fix PR

OK, I added the "defect" tag to this PR.

@WillTirone WillTirone deleted the fix_12952 branch May 1, 2022 15:29
@jarrodmillman jarrodmillman mentioned this pull request Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WarrenWeckesser WarrenWeckesser WarrenWeckesser approved these changes

Assignees
No one assigned
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.stats
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

[Documentation question] Would it be more precise to specify p in [0,1] for probability distributions?
3 participants
@WillTirone @WarrenWeckesser @josef-pkt