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

Port remaining distributions to v4 #4686

Closed
26 tasks done
twiecki opened this issue May 12, 2021 · 9 comments · Fixed by #5382
Closed
26 tasks done

Port remaining distributions to v4 #4686

twiecki opened this issue May 12, 2021 · 9 comments · Fixed by #5382
Labels
Milestone

Comments

@twiecki
Copy link
Member

twiecki commented May 12, 2021

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
Copy link
Member

ricardoV94 commented May 12, 2021

@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: #4518 (comment)

@pymc-devs pymc-devs deleted a comment from ricardoV94 May 12, 2021
@farhanreynaldo
Copy link
Contributor

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

@twiecki
Copy link
Member Author

twiecki commented May 12, 2021 via email

@ricardoV94
Copy link
Member

ricardoV94 commented May 12, 2021

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@twiecki
Copy link
Member Author

twiecki commented Jun 17, 2021

@AlexAndorra I added it to the list.

@twiecki
Copy link
Member Author

twiecki commented Jan 30, 2022

opened this issue on May 12, 2021

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

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 a pull request may close this issue.

7 participants