-
-
Notifications
You must be signed in to change notification settings - Fork 987
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
fixed NaN with in log_prob
corr < -1e-8 for SineBivariateVonMises
#3165
fixed NaN with in log_prob
corr < -1e-8 for SineBivariateVonMises
#3165
Conversation
Do you want to clip like in numpyro? |
@fehiepsi yes, but couldn't find the float epsilon of a tensor in the docs. |
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.
Thanks Ola. It looks like there are a couple of typos in the docstring of weighted_correlation
. Could you fix them: weightd_correlation
and weigthed_corr
correlation = ( | ||
weighted_correlation * sqrt_(phi_concentration * psi_concentration) | ||
(weighted_correlation * sqrt_(phi_concentration * psi_concentration)).clamp(min=eps) | ||
+ 1e-8 |
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.
Do we need to clamp and and 1e-8 here? I think we address numerical issues in norm_const
property.
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.
If you remove 1e-8
here. Could you also remove it in numpyro for consistency?
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.
Yeah, I guess we don't need either. I'll fix it in NumPyro too.
@@ -135,9 +136,10 @@ def norm_const(self): | |||
(self.phi_concentration, self.psi_concentration), dim=-1 | |||
).view(-1, 2) | |||
m = torch.arange(50, device=self.phi_loc.device).view(-1, 1) | |||
eps = torch.finfo(corr.dtype).eps |
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 think you can use tiny
. Its log is still finite.
Fixes
NaN
inlog_prob
when the correlation is below zero inSineBivariateVonMises
. Removes correlation bias (+1e-8
) in normalization constant.See PR NumPyro#1515 and issue NumPyro#1511.