-
-
Notifications
You must be signed in to change notification settings - Fork 986
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
fix _loc_scale method in AutoMultivariateNormal #3233
Conversation
pyro.sample("y", dist.LogNormal(0.0, 1.0)) | ||
pyro.sample("z", dist.Beta(2.0, 2.0).expand([2]).to_event(1)) | ||
pyro.sample("x", dist.Normal(0.0, 1.0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move "x" so that it's not in the trivial top-left of scale_tril
position
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise this test would still pass in master : )
if hasattr(auto_class, "get_posterior"): | ||
posterior = guide.get_posterior() | ||
posterior_scale = posterior.variance[-1].sqrt() | ||
q = guide.quantiles([0.158655, 0.8413447]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume [0.158655, 0.8413447] == Normal(0, 1).icdf(torch.tensor([-1.0, 1.0]))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can replace with this if you prefer
guide.quantiles(dist.Normal(0, 1).cdf(torch.tensor([-1.0, 1.0])).data.numpy().tolist())
clunky because quantiles
wraps in a tensor which raises a user warning if the input is a tensor....
UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
alternatively can add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we need a correlated model to test this change? E.g.
def model():
w = pyro.sample("w", dist.Normal(0.0, 1.0))
x = pyro.sample("x", dist.Normal(x, 1.0))
i don't think so. we just need to make sure we're computing the correct diagonal element of the full covariance matrix. so as long as we're doing that in a case where that computation is non-trivial (because e.g. it involves off-diagonal elements of |
@fritzo afaict only |
My concern is that in the existing test, all those off-diagonal elements happen to be zero, so the old and new formulae happen to agree. Am I missing something? |
the
|
addresses #3232
cc @fritzo