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

Fix exponential and gamma logp / random link #4576

Merged
merged 1 commit into from
Apr 4, 2021

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Mar 27, 2021

After merging #4548, the Exponential and Gamma distributions still have a mismatch between the logp and random parametrizations.

I still think it is more straightforward to keep relying on the tests in test_distributions_random.py while we refactor the distributions into V4 as it easily identifies this type of issues. There is already an open issue to refactor these tests #4554, so it will not be forgotten.

For the discussion into the Exponential and Gamma changes see my previous comments:

  1. Exponential: Fix different / alternative parametrizations between RandomOps and Logp methods #4548 (comment)
  2. Gamma: Fix different / alternative parametrizations between RandomOps and Logp methods #4548 (comment)

There seems to be a problem with the Categorical (test_distributions_random::TestScalarParameterSamples::test_categorical_random), but perhaps it was addressed by pymc-devs/pytensor#351?

Copy link
Contributor

@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.

We do not need to create that new RandomVariable or add a new optional parameterization at this time.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Mar 27, 2021

What do you suggest instead? Shall we leave the distributions not matching parameters on the logp and random sides?

For the exponential even if we don't provide it as an option, we still need to use mu behind the scenes since that's how the aesara op is parametrized.

The gamma right now requires changing the aesara op one way or another as explained in the comment above.

@brandonwillard
Copy link
Contributor

brandonwillard commented Mar 27, 2021

What do you suggest instead? Shall we leave the distributions not matching parameters on the logp and random sides?

We only need to keep the Distribution interface, so, inside Exponential.dist, we can at.inv the parameter and update the corresponding logp and logcdf functions. The latter two functions are not called directly, so they only need to correspond to the RandomVariable parameterizations.

@michaelosthege michaelosthege modified the milestone: vNext (4.0.0) Mar 27, 2021
@ricardoV94 ricardoV94 force-pushed the fix_exponential_gamma_random branch 3 times, most recently from 7c2edbe to 6a86c34 Compare March 29, 2021 07:57
@ricardoV94
Copy link
Member Author

I removed the explicit new parametrization of the exponential and forced the gamma to work with the parameter inversion on the aesara side. The last one still feels weird, but if that's how we want to keep it, so be it.

@brandonwillard
Copy link
Contributor

I rebased this branch onto the update v4.

@ricardoV94
Copy link
Member Author

Thanks!

@brandonwillard brandonwillard force-pushed the fix_exponential_gamma_random branch 3 times, most recently from cfe6599 to d388c83 Compare March 29, 2021 23:06
@ricardoV94
Copy link
Member Author

I changed the exponential equal assertion to almost_equal as it was failing on float32. The tests should pass now.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Mar 30, 2021

Unrelated to this PR this test failed:
test_sampling.py::test_exec_nuts_init

@ricardoV94 ricardoV94 deleted the fix_exponential_gamma_random branch April 22, 2021 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants