Skip to content

Conversation

@fehiepsi
Copy link
Member

@fehiepsi fehiepsi commented Feb 12, 2021

Resolves #895, #894, #914. The implementation only supports symmetric distributions because we can leverage the symmetry to address numerical issues that are discussed at probtorch for the lower bound > 5. The issue is cdf/icdf suffers from precision errors at the right tail (e.g. Normal().cdf(6.) = 1.). Thanks to the symmetry, we can transform the right tail to the left tail, which has better precision (e.g. Normal().cdf(-6.) = 9.865896e-10).

TODO

  • Add student T cdf/icdf
  • Support truncated from above
  • Add tests for cdf/icdf
  • Add tests for TruncatedDistribution
  • Make pytree test pass

@fehiepsi
Copy link
Member Author

@fritzo Could you help me review this PR? Pls consider this as a low priority PR. A nice thing is your gof helps me fix several mistakes in the sample method. ;)

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface looks great! I haven't checked math of .cdf(),.icdf() but I would feel more confident if we added some simple algebraic tests independent of SciPy, testing with a large grid of random values.

Comment on lines 1117 to 1120
# if low < loc, returns cdf(high) = 1; otherwise returns 1 - cdf(high) = 0
loc = self.base_dist.loc
sign = jnp.where(loc >= self.low, 1., -1.)
return 0.5 * (1 + sign)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not simply

return jnp.where(self.low < self.base_dist.loc, 1., 0.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, thanks

quantiles = random.uniform(random.PRNGKey(1), (100,) + d.shape())
try:
if d.shape() == ():
rtol = 1e-3 if jax_dist is dist.StudentT else 1e-5
Copy link
Member Author

@fehiepsi fehiepsi Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rtol=1e-5 fails for one StudentT test. I guess the precision of betainc/its grad is not good enough or samples got extreme values.

@fritzo fritzo merged commit ff691be into pyro-ppl:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for TruncatedDistribution

2 participants