Skip to content

Wrap tfp distributions as pm RVs #41

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

Merged
merged 7 commits into from
Jan 14, 2019
Merged

Conversation

eigenfoo
Copy link
Member

@eigenfoo eigenfoo commented Jan 12, 2019

(Following up @twiecki's suggestion)

This PR wraps tfp.distributions as pm.RandomVariables. We enter the list of tfp distributions we want as strings in __all__, and the for loop at the end of the module should define the necessary classes.

Some notes:

  • I assumed that we would name our RVs the exact same name as the tfp distributions (e.g., it's now Chi2, not ChiSquared).
  • I made the list of distributions by just comparing what RVs we have in PyMC3 right now. I'll double check this list later today.

@eigenfoo eigenfoo added the enhancement New feature or request label Jan 12, 2019
@eigenfoo eigenfoo self-assigned this Jan 12, 2019
@ColCarroll
Copy link
Member

Cool! I guess since you set __all__, this allows tab completion?

I might use

setattr(sys.modules[__name__], dist_name, getattr(tfp.distributions, dist_name))

because I like setattr and getattr better. Would that even work? This suggests they are similar, and gives no prohibition against either. I guess take this as an observation that there's another way to do it, then!

Looks good to merge, but I'll give it a few more hours in case there's input.

@eigenfoo
Copy link
Member Author

@ColCarroll That does indeed work! I like getattr and setattr better: I didn't know that they worked at the module level. I've changed it to do that.

And no, I manually typed out the distributions, since I use vim with very few plugins 😛

@eigenfoo
Copy link
Member Author

eigenfoo commented Jan 12, 2019

Looking carefully, tfp does not implement the following distributions, which are currently supported in PyMC3. There are some fairly important ones here: we'll need to decide what we want to do there (i.e. implement the RVs ourselves, submit PRs to tfp, or drop support for the obscure distributions). We should go ahead and merge this PR, and deal with the rest of these distributions in subsequent PRs.

Continuous:

  • Flat
  • HalfFlat
  • SkewNormal
  • HalfStudentT
  • Weibull
  • Wald
  • ExGaussian
  • LogitNormal
  • Interpolated

Discrete:

  • BetaBinomial
  • ZeroInflatedBinomial
  • ZeroInflatedPoisson
  • ZeroInflatedNegativeBinomial
  • DiscreteUniform
  • Constant (EDIT: actually, tfp does have this, but it's called tfp.distributions.Deterministic... which may be problematic for us 😝)
  • DiscreteWeibull
  • OrderedLogistic

Multivariate:

  • MatrixNormal
  • KroneckerNormal
  • LKJCholeskyCov and LKJCorr (there's just an "LKJ" distribution, though)

Timeseries:

  • All Timeseries distributions

@ColCarroll ColCarroll merged commit e04b3b6 into master Jan 14, 2019
@ColCarroll
Copy link
Member

This will be nice, not adding them piecemeal. Thanks, @eigenfoo!

@eigenfoo eigenfoo deleted the wrap-tfp-distributions branch January 14, 2019 01:59
@majidaldo
Copy link
Contributor

What happens if there is a distribution that can't be based of a (existing) TFP distribution? TFD Distributions have so many methods. In PyMC3 you could at least attempt to provide your own .log_p() and sample().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants