search

pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.7k stars 2.01k forks source link

Port remaining distributions to v4 #4686

Closed twiecki closed 2 years ago

twiecki commented 3 years ago

There are still various continuous distributions missing for v4 that need to get ported. Here is an example of how to do this port for where aesara does not provide a RandomVariable (which is all of the currently missing ones): https://github.com/pymc-devs/pymc3/blob/v4/pymc3/distributions/continuous.py#L2648

Here is a list of the missing continuous ones:

And the missing multivariate ones:

Would be very important to add these to get an alpha out.

See @ricardoV94's comment below for a step-by-step guide on how to port a distribution.

ricardoV94 commented 3 years ago

@ricardoV94 Thanks, edited the above. If you have time, perhaps you could write a short list of steps of what to do to port them. (Unless you want to take these on yourself :)).

This PR illustrates well with a single distribution refactoring I think: https://github.com/pymc-devs/pymc3/pull/4615/files. What is needed is:

  1. Create a new RV op by inheriting from aesara RandomVariable, porting the code in the old random method to the rng_fn classmethod of the new RandomVariable.
  2. Add the new (instance) of the RandomVariable to the rv_op field in the respective PyMC3 distribution.
  3. Port any initialization logic in the old __init__ to the new dist classmethod
  4. Refactor logp and logcdf to expect arguments in this order: value, arg1, arg2, ... argn
  5. Re-enable tests in test_distributions.py
  6. Create new test in test_distributions_random.py as seen here, and remove old BaseTestCase in that file.
  7. Re-enable other tests that depended on this distribution in other test files.

Importantly, if there are multiple parametrizations, one will have to be set as the default in the dist classmethod in step 3. This will be the one that the RandomVariable is defined in terms of. If the random methods and logp/logcdf methods use different parametrizations, they will have to be converted to the ones that the RandomVariable is defined in terms of in step 3, and converted back to the logp and logcdf methods in step 4.

More details can be found here: https://github.com/pymc-devs/pymc3/issues/4518#issuecomment-806236360

farhanreynaldo commented 3 years ago

I would like to help on this issue. Should we create a separate PR for each distribution?

twiecki commented 3 years ago

No you can do multiple in one go, just mark the ones you are working on.

On Wed, May 12, 2021, 13:28 Farhan Reynaldo @.***> wrote:

I would like to help on this issue. Should we create a separate PR for each distribution?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc3/issues/4686#issuecomment-839695709, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFETGDPFKEEQJRE2ZEKOVDTNJQ4ZANCNFSM44X5P5LQ .

ricardoV94 commented 3 years ago

I would like to help on this issue. Should we create a separate PR for each distribution?

That might be too much. You can group a couple (or all) in a single PR.

I would suggest you start with the univariate distributions. The multivariate ones will probably be a bit more tricky.

ricardoV94 commented 3 years ago

@brandonwillard what should we do with the flat and halfFlat distributions that don't have a random method? Should we create a RandomVariable whose rng_fn returns the same ValueError("Cannot sample from Flat distribution") as in V3?

brandonwillard commented 3 years ago

@brandonwillard what should we do with the flat and halfFlat distributions that don't have a random method? Should we create a RandomVariable whose rng_fn returns the same ValueError("Cannot sample from Flat distribution") as in V3?

Yeah, we can do that for now.

AlexAndorra commented 3 years ago

Was LKJCholeskyCov refactored? I don't see it in the list

twiecki commented 3 years ago

@alexandorra I added it to the list.

twiecki commented 2 years ago

opened this issue on May 12, 2021

Holy cow, can't believe we finally got them all! 🥳