Porting Sine Bivaraite von Mises from Pyro #1063
Conversation
…vm_dist # Conflicts: # numpyro/distributions/directional.py
There was a problem hiding this comment.
Thanks, @OlaRonning! I haven't checked the math because this is your port from Pyro. I just have some comments on shape sematics.
There was a problem hiding this comment.
Hi @OlaRonning, I missed several points in the first pass. Here are some of the new ones. :)
|
Thanks for the review @fehiepsi. |
…vm_dist # Conflicts: # numpyro/distributions/directional.py # test/test_distributions.py
|
|
||
| arg_constraints = { | ||
| "phi_loc": constraints.circular, | ||
| "psi_loc": constraints.circular, |
There was a problem hiding this comment.
I feel that we can relax this constraint - even with real constraints, we still have valid distributions. What do you think?
There was a problem hiding this comment.
I like the circular constraint because it is explicit; what is the advantage of using real constraint?
There was a problem hiding this comment.
It is just unnecessary imo (e.g. we can allow users to put in angles from pi to 2pi or even 10pi). But it is up to you.
Translating Sine Bivariate von Mises Distribution from Pyro; phi marginal is sampled similarly to von Mises distribution to accommodate for immutable arrays.