paul-buerkner / brms

brms R package for Bayesian generalized multivariate non-linear multilevel models using Stan
https://paul-buerkner.github.io/brms/
GNU General Public License v2.0
1.27k stars 182 forks source link

use von_mises_lpdf directly instead of von_mises2_lpdf after next stan update #1633

Closed venpopov closed 1 month ago

venpopov commented 6 months ago

Currently the von_mises() family is not vectorized if kappa is predicted, because the von_mises_lpdf is numerically unstable for large kappas, and you use the normal approximation for kappa > 100, which requires an if else check.

I have submitted a PR in stan math library that improves the stability of the von_mises_lpdf and which will make the von_mises_lpdf2 unneccessary, as I describe in the issue here.

Once that is accepted and released, you could simplify the von mises family, which my tests show it speeds it up, and also avoids scale=0 warning when 1/sqrt(kappa) passed to normal_lpdf is 0. Some tests described here at the end.

It's probably going to be a while until the next release and also that will work just for the cmdstar backend until rstan catches up, but I'm nothing it here for when the time is right.

of2 commented 1 month ago

+1 boosting this - thanks for fixing this issue in Stan @venpopov! Keen for it to be updated in BRMS - after seeing your threads on the Stan forums + the pull request here https://github.com/stan-dev/math/pull/3036 I was able to resolve some hiccups in my models by editing the stan code generated by BRMS to use von_mises_lpdf instead of von_mises2_lpdf

paul-buerkner commented 1 month ago

Let's just change it now :-)

paul-buerkner commented 1 month ago

Done :-)