Closed spinkney closed 2 years ago
I can try... this involves adding loglogistic_x.hpp functions to prim/prob, right?
That is correct. Thanks!
No problem. Two questions:
logistic_x is probably a good starting point I would say.
Maybe take a look at a recent PR adding a new distribution to see the requirements: https://github.com/stan-dev/math/pull/2271
Tagging @bbbales2 that has been the designated reviewer for these kind of PRs recently.
@adamhaber am back! The distribution functions are pretty involved. If you wanna do em', yeah have a look at #2271.
Reverse mode implementations of some existing functions might be an easier place to start though (less moving parts). Can dig up a couple good ones if you want.
@bbbales2 thanks for the suggestion, I'll have a look.
Reverse mode implementations of some existing functions might be an easier place to start though (less moving parts). Can dig up a couple good ones if you want.
You mean as a separate, simpler PR "to start with"?
@adamhaber yeah yeah. The distributions are tough. I was trying to write down some notes on how to write functions using the new autodiff, but it's been a few days and I haven't finished so don't wanna keep you waiting.
diag_pre_multipy
and diag_post_multiply
both could use reverse mode autodiff implementations.
I think use elt_multiply
as a template.
Prim implementation here Rev implementation here Prim tests here Autodiff tests here
That's not much information -- there's a lot of weird stuff in those functions. Hit me up on the slack when stuff starts acting weird (cuz it almost certainly will): https://discourse.mc-stan.org/t/mc-stan-community-slack/2410
This feeds into the stuff we're doing converting everything to support the varmat autodiff types, but also some other stuff, so will be good things to know.
@bbbales2 I can do the reverse mode autodiff implementations of diag_pre_multiply
and diag_post_multiply
, if @adamhaber didn't already start on that.
@gregorp90 go for it. Probably just start with one (and then the second one will be easy). @adamhaber is working on a different function.
Great, thanks!
Hi, is anyone working on this @bbbales2? Maybe @adamhaber? If not, then I can try it out.
No one has commented here that they are working on it or opened a Work-in-progress PR, so I would say go for it.
Ok great, thank you!
I started working on some docs for adding new distributions, got about halfway through it today (and tbh stopped at the hard part of explaining all the whistles and bells) but I'll try to get this finished tomorrow. I'm using loglogistic_cdf
as an example since everything we do there should work with the other functions.
You can checkout the branch below and run make doxygen
to make the site locally
@SteveBronder Thanks, I skimmed through it and it looks very helpful. I started implementing the loglogistic_lpdf
, so I will definitely go through this help file in more detail and let you know if I have any comments.
Just a note, I guess you decided not to use loglogistic_cdf
but normal_lpdf
instead? Because I don't see the loglogistic_cdf
in the help file I generated.
I also looked at the "Testing New Distributions" section of this help file. What I miss there are the instructions to add GENERATE_DISTRIBUTION_TESTS=true
to the make/local file. Maybe you could add a note on this, if you are updating these help files. Tadej told me I need this. If he didn't I would probably lose a couple of hours in trying to figure out why the testing isn't working, so it might be helpful to mention it.
Just a note, I guess you decided not to use loglogistic_cdf but normal_lpdf instead? Because I don't see the loglogistic_cdf in the help file I generated.
Oh yeah sorry so I changed it up because I could just use some of the examples from the Stan math paper for normal_lpdf
I also looked at the "Testing New Distributions" section of this help file. What I miss there are the instructions to add GENERATE_DISTRIBUTION_TESTS=true to the make/local file. Maybe you could add a note on this, if you are updating these help files. Tadej told me I need this. If he didn't I would probably lose a couple of hours in trying to figure out why the testing isn't working, so it might be helpful to mention it.
Good catch! I didn't see that is not set for windows and I'll add that note to the distribution tests docs
should I close this now that @gregorp90's pr is merged?
We can leave it open until the cdf etc is added
I'll start on the other functions as soon as I have some time. It will probably go a bit quicker now.
Thanks all for working on this - it will be very useful! Just a naive question from a newcomer - when do you think this will be available in stan / cmdstan / cmdstanr? :-)
The loglogistic_lpdf should be available for the next release coming at the end of January. The cdf, lcdf and other functions typically available with distributions will not be available though.
thx @rok-cesnovar ! just to understand: the other functions are not ready? since I saw that #2488 implemented some?
Oh yeah, the loglogistic_cdf
one was done. So this is only missing the loglogistic_lccdf
and loglogistic_lcdf
before this issue can be closed. loglogistic_lpdf, loglogistic_cdf and loglogistic_rng should be available in 2.29 end of January.
Closing this as we opened a more general CDF function issue: #2657
Here's Stan code I put together. The
rng
function includes lower and upper bounds because I needed a truncated distribution but can be removed to be consistent with otherrng
functions.