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

Add JAX implementation for InvGammaRV #1480

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

PaulScemama
Copy link

@PaulScemama PaulScemama commented Mar 19, 2023

This PR is a draft to close #1368. Furthermore, I had to change the .pre-commit-config.yaml slightly to pass the mypy check. This bug has been discussed in #1474 (reply in thread).

@brandonwillard @rlouf let me know what you both think regarding the types-setuptools issue. If it hasn't affected anyone else then I would assume you wouldn't want the change to the .pre-commit-config.yaml.

Here are a few important guidelines and requirements to check before your PR can be merged:

  • There is an informative high-level description of the changes.
  • The description and/or commit message(s) references the relevant GitHub issue(s).
  • pre-commit is installed and set up.
  • The commit messages follow these guidelines.
  • The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
  • There are tests covering the changes introduced in the PR.

EDIT:

Something that I didn't realize in the beginning...from wolfram:

"the inverse gamma distribution with shape parameter $\alpha$ and scale parameter $\beta$ is the distribution followed by the inverse of a gamma distribution with shape parameter $\alpha$ and scale parameter $\color{red} 1 / \beta$."

Could be useful to say something like:

"the InvGamma(shape, scale) is equivalent to taking the reciprocal of samples from a Gamma(shape, 1 / scale) distribution" in the docs.

@brandonwillard brandonwillard added JAX Involves JAX transpilation random variables Involves random variables and/or sampling labels Mar 20, 2023
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #1480 (0b66d2a) into main (4a687c0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0b66d2a differs from pull request most recent head eda2f50. Consider uploading reports for the commit eda2f50 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1480   +/-   ##
=======================================
  Coverage   75.06%   75.06%           
=======================================
  Files         194      194           
  Lines       50091    50101   +10     
  Branches    12096    12097    +1     
=======================================
+ Hits        37600    37610   +10     
  Misses      10170    10170           
  Partials     2321     2321           
Impacted Files Coverage Δ
aesara/link/jax/dispatch/random.py 100.00% <100.00%> (ø)

@PaulScemama PaulScemama changed the title Draft: Add JAX implementation for InvGammaRV Add JAX implementation for InvGammaRV Mar 23, 2023
@brandonwillard
Copy link
Member

"the InvGamma(shape, scale) is equivalent to taking the reciprocal of samples from a Gamma(shape, 1 / scale) distribution" in the docs.

Yes, definitely.

@brandonwillard
Copy link
Member

This PR is a draft to close #1368. Furthermore, I had to change the .pre-commit-config.yaml slightly to pass the mypy check. This bug has been discussed in #1474 (reply in thread).

Is that still an issue now that #1482 is merged?

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

I pushed some small changes and squashed; should be ready to merge after passing.

tests/link/jax/test_random.py Outdated Show resolved Hide resolved
@brandonwillard brandonwillard enabled auto-merge (rebase) March 23, 2023 22:20
@PaulScemama
Copy link
Author

This PR is a draft to close #1368. Furthermore, I had to change the .pre-commit-config.yaml slightly to pass the mypy check. This bug has been discussed in #1474 (reply in thread).

Is that still an issue now that #1482 is merged?

It is not an issue anymore.

@brandonwillard brandonwillard merged commit 4e01892 into aesara-devs:main Mar 23, 2023
@brandonwillard
Copy link
Member

Thanks a lot, @PaulScemama!

@PaulScemama
Copy link
Author

Thank you! @brandonwillard I learned a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JAX Involves JAX transpilation random variables Involves random variables and/or sampling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JAX implementation for InvGammaRV
3 participants