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 numerical tests to transformations #542

Closed
yebai opened this issue Sep 15, 2018 · 8 comments
Closed

Add numerical tests to transformations #542

yebai opened this issue Sep 15, 2018 · 8 comments

Comments

@yebai
Copy link
Member

yebai commented Sep 15, 2018

The current test file for transform.jl only test the functionality of logpdf_with_trans

logpdf_with_trans(dist, x, true)

We need to add correctness tests for logpdf, e.g.

Distributions.logpdf(d, x) == Turing.logpdf_with_trans(d, link(x), true)
@willtebbutt
Copy link
Collaborator

Good catch. I completely forgot to sort that out when I was refactoring. Will sort it out now.

@willtebbutt
Copy link
Collaborator

#543 now resolves most of these issues, but there remain a couple of outstanding issues:

  1. A few distributions (Dirichlet, Wishart, InverseWishart) don't have tests to ensure that their link functions are consistent with their logpdf_with_trans implementation. Due to the way that we transform between the constrained and unconstrained representations the Jacobian of the transformation is singular. This isn't a fundamental issue, but I'm not completely sure what the best way to resolve it is beyond directly hacking away at stuff.
  2. The logpdf of the Kolmogorov distribution returns -Inf for x close to zero. I assume this is a numerical stability issue, so I've disabled testing for that for now as it messes everything up, and is probably on the Distributions.jl end.

@yebai
Copy link
Member Author

yebai commented Sep 17, 2018

A few distributions (Dirichlet, Wishart, InverseWishart) don't have tests to ensure that their link functions are consistent with their logpdf_with_trans implementation. Due to the way that we transform between the constrained and unconstrained representations the Jacobian of the transformation is singular. This isn't a fundamental issue, but I'm not completely sure what the best way to resolve it is beyond directly hacking away at stuff.

I did some eye-ball tests a while ago and didn't find any obvious errors.

@willtebbutt
Copy link
Collaborator

I did some eye-ball tests a while ago and didn't find any obvious errors.

Sure, I agree that it might be fine, but we really should be testing this stuff heavily as, as you point out, the numerical stability of all of our HMC-related code depends on it quite heavily.

@xukai92
Copy link
Member

xukai92 commented Sep 17, 2018

Due to the way that we transform between the constrained and unconstrained representations the Jacobian of the transformation is singular. This isn't a fundamental issue, but I'm not completely sure what the best way to resolve it is beyond directly hacking away at stuff.

Will a numerical approximation of Jacobian work here?

@willtebbutt
Copy link
Collaborator

A numerical approximation would have the same issues, and would just introduce numerical errors vs the AD-based one. We're representing a vector that has D-1 degrees of freedom using a D-dimensional vector, so the Jacobian of the transform will be singular by definition. The solution is to re-write the tests slightly so that they're aware of this property, and to compute the Jacobian slightly differently. It's not particularly hard, it's just a bit awkward.

@xukai92
Copy link
Member

xukai92 commented Sep 17, 2018

I got it! Thanks for the explanation!

@yebai
Copy link
Member Author

yebai commented Sep 28, 2018

Closed in favour of TuringLang/Bijectors.jl#5

@yebai yebai closed this as completed Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants