Skip to content

Updates to pymc.Hypergeometric docstrings #5488

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

Merged
merged 6 commits into from
Mar 3, 2022
Merged

Updates to pymc.Hypergeometric docstrings #5488

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21498ef
Added bounds to the docstring of the Hypergeometric dist
cuchoi Feb 18, 2022
6250180
Merge pull request #1 from cuchoi/cuchoi-hypergeometric-docstring-update
cuchoi Feb 18, 2022
063440c
Updated pymc.Hypergeometric docstring
cuchoi Feb 18, 2022
413ed0e
Added array_like types for parameters
cuchoi Feb 19, 2022
30898b9
Added array_like types for parameters
cuchoi Feb 19, 2022
2827e57
Update pymc/distributions/discrete.py
OriolAbril Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions pymc/distributions/discrete.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,11 +972,11 @@ class HyperGeometric(Discrete):
Parameters
----------
N : integer
Copy link
Member

Choose a reason for hiding this comment

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

are scalars the only valid type for all 3 arguments? cc @ricardoV94 ? (no idea who knows, feel free to tag someone else)

Copy link
Member Author

Choose a reason for hiding this comment

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

They should be only integers: https://en.wikipedia.org/wiki/Hypergeometric_distribution

Copy link
Member

Choose a reason for hiding this comment

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

but maybe an array of integers is also supported? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

logcdf does say:

        # logcdf can only handle scalar values at the moment
        if np.ndim(value):
            raise TypeError(
                f"HyperGeometric.logcdf expects a scalar value but received a {np.ndim(value)}-dimensional object."
            )

Copy link
Member Author

Choose a reason for hiding this comment

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

But maybe logp supports it?

Copy link
Member Author

@cuchoi cuchoi Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can I use a different format to specify the array types? Example integer, array_like[integer] or TensorVariable[integer]?

Copy link
Member

Choose a reason for hiding this comment

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

no, numpydoc formatting is very clear in the array of type format

Copy link
Member

Choose a reason for hiding this comment

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

We decided to use tensor_like of integer and we'll add its definition on the glossary so we have a type that covers scalars, arrays, aesara tensorvariable and randomvariable

Copy link
Member Author

@cuchoi cuchoi Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Noted! Picked integer, array_like or TensorVariable

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 late review. We decided to go with tensor_like of integer which will get rendered as a link to our glossary where we have an explanation on what inputs are accepted for arguments documented like this: https://docs.pymc.io/en/latest/glossary.html#term-tensor_like

Total size of the population
Total size of the population (N > 0)
k : integer
Number of successful individuals in the population
Number of successful individuals in the population (0 <= k <= N)
n : integer
Number of samples drawn from the population
Number of samples drawn from the population (0 <= n <= N)
"""

rv_op = hypergeometric
Expand Down Expand Up @@ -1004,6 +1004,10 @@ def logp(value, good, bad, n):
value : numeric
Value(s) for which log-probability is calculated. If the log probabilities for multiple
values are desired the values must be provided in a numpy array or Aesara tensor
good : integer
Number of successful individuals in the population. Alias for parameter :math:`k`.
bad : integer
Number of unsuccessful individuals in the population. Alias for :math:`N-k`.

Returns
-------
Expand Down Expand Up @@ -1042,8 +1046,14 @@ def logcdf(value, good, bad, n):

Parameters
----------
value: numeric
value : numeric
Value for which log CDF is calculated.
good : integer
Number of successful individuals in the population. Alias for parameter :math:`k`.
bad : integer
Number of unsuccessful individuals in the population. Alias for :math:`N-k`.
n : integer
Number of samples drawn from the population (0 <= n <= N)

Returns
-------
Expand Down