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

Added Binomial distribution #101

Merged
merged 8 commits into from
Aug 6, 2024

Conversation

MoritzNeuberger
Copy link
Contributor

Source code is inspired by the scipy implementation.

@HDembinski
Copy link
Owner

Very nice contribution at first glance, sorry I missed this. I don't get notified by Github when a PR is created, but I get a notification if you ping me explicitly with @HDembinski


@_jit(2, cache=False)
def _logpmf(k, n, p):
T = type(n)
Copy link
Owner

Choose a reason for hiding this comment

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

I think you want the type of p here, not n

n : int
number of trails.
p : float
success probability for each trail.
Copy link
Owner

Choose a reason for hiding this comment

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

trial

@HDembinski
Copy link
Owner

HDembinski commented Aug 6, 2024

There were a couple of other issues, which I fixed here, that had quite far-reaching ripples into the code.

  • The binom functions should accept both k and n arrays, while in your implementation n was a scalar. The idea that only the first argument is an array is deeply rooted in the design of the library, though, so it is natural to design it like this. To support the more flexible case, I revised the _jit utility to support functions with more than one array argument.
  • You cannot compute the cdf by summing up the pmf, because that does not scale to large n. The correct way is to use the incomplete beta function.

@HDembinski HDembinski merged commit 3302ff0 into HDembinski:main Aug 6, 2024
3 checks passed
@MoritzNeuberger
Copy link
Contributor Author

Oh wow, you are right, that's pretty cool!

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.

2 participants