| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…i2114/FranziStan into issue-2682-Add-7-parameter-DDM-PDF Changes from FranziStan
|
I believe you'll want @SteveBronder, @andrjohns, or @rok-cesnovar to have a closer look. I think we'll want to discuss what's the best name for these. I'm not an expert in these but I'm curious how the 5 parameter one different than the wiener_lpdf already in stan? It's 4 parameters but then we separate in a model whether the outcome hit the lower or upper bound. |
Sorry, something went wrong.
The 5 parameter model incorporates the variability in the drift rate parameter. This is not possible in wiener_lpdf. You can set this variability to 0, then you obtain the 4 parameter model which has the same output as wiener_lpdf, but since we have all partial derivatives implemented, the whole procedure iscomputationally much more efficient. We also need the wiener5 functions for the calculation of the wiener7 functions. |
Sorry, something went wrong.
I can review this tomorrow |
Sorry, something went wrong.
Very cool! When we expose this to Stan we can just use wiener_lpdf and have different signatures for each type. Do we even want to keep the current wiener_lpdf if this one is superior? If you get up the energy to add the lcdfs and the rng functions, I'd love to have those in. I have a model that needs the lcdf and lccdf I've been wanting to test. I did code up the 4 parameter _rng function (#2801) in Stan but haven't had the time to put it in. |
Sorry, something went wrong.
Thanks a lot for your review and your ideas. Putting it together with wiener_lpdf sounds good to me. I think as well that replacing the current wiener_lpdf implementation is worth considering. @Franzi2114 or I could do a comparison to be able to put a number on the performance difference between the implementations. |
Sorry, something went wrong.
Yeah, thanks @spinkney! That's true. When setting all variabilities to 0 and calling the function with wiener_full_lpdf(a,v,w,t0,0,0,0) the four parameter model will be computed. Therefore, the wiener_lpdf() gives no advantages anymore. We have not yet implemented the lcdf- and rng-functions. We have to check how much effort it is to bring them to Stan. But we will check this and may contribute them later in a separate PR. First, we wanted to add the lpdf-functions. |
Sorry, something went wrong.
Yes, I agree! It's best to do these in separate prs. |
Sorry, something went wrong.
|
@Franzi2114 from my understanding this PR comprises three main contributions:
Can you split this into three PRs rather than all together? That will make it much easier to ensure that each component is fully tested, and also gives a bit of a cleaner development history for the repo. Thanks! |
Sorry, something went wrong.
Hi @andrjohns, thanks for your input! Yes, we can split our PR into three PRs. I will refer to the same issue in the branchnames (Feature/issue 2682 operands and partials 8, and Feature/issue2682 hcubature). Up to now, we don't have separate tests for hcubature. The integration is checked in the tests for wiener_full_lpdf(). So we will add separate tests and do a third PR. |
Sorry, something went wrong.
|
Hey @andrjohns, I am confused. I only merged the develop branch (where the new operands_and_partials_8 routine is integrated now) into this branch and the continuous-integration test fails at a point I do not understand. Did I do anything wrong? |
Sorry, something went wrong.
|
@serban-nicusor-toptal would you mind restarting the Threading tests for this Jenkins run? Is it possible to give my account permissions to restart runs as well? So you don't have to get bothered with pings everytime |
Sorry, something went wrong.
There was a problem hiding this comment.
Not sure why there are Jenkinsfile changes still showing in the diff, since I've git-reverted the file to match develop (and the changes in the diff are just the current state of develop). Bizarre.
Anyway, saw that there was an outstanding review comment from me, so marking this as approved. Looks like this is all good to go!
Feel free to merge once the current test run finishes up. Thanks for sticking through this marathon PR, your contribution (and patience) is very much appreciated!
Sorry, something went wrong.
Jenkins Console Log Machine information No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: G++: Clang: |
Sorry, something went wrong.
|
@Franzi2114 feel free to click the merge button at your convenience :) Your contribution is very appreciated! When you have a moment would you mind writing the Stan docs for the new 5 and 7 parameter wiener distributions in the page below? |
Sorry, something went wrong.
Great! Wow, I am so happy that the PR is eventually completed! ^^ However, I cannot press the merge button as I am not authorized to do this. So, could one of you merge the PR? |
Sorry, something went wrong.
|
I think we can add @Franzi2114 to let him do the honors himself |
Sorry, something went wrong.
|
Oh! I'm terribly sorry! idk when we changed that? I think it's nice for contributors to click merge on their own PRs. Let me see tmrw how to change that |
Sorry, something went wrong.
|
@Franzi2114 I just made you a member of the Stan math repo :) You may have to check your email to turn it on. But you should have merge permissions now! |
Sorry, something went wrong.
|
Hm ... I accepted the invitation and are now member of the repo. I reloaded this page and signed out and in again. But still the message is 'You are not authorized to merge this pull request.' Seems more tricky to do the last step than expected ^^ |
Sorry, something went wrong.
|
@Franzi2114 sorry try one more time. Thought I gave you the right access but it was one less than you needed |
Sorry, something went wrong.
|
Now there popped up another box with a rocket saying 'This branch has not been deployed', but I still cannot merge the PR. |
Sorry, something went wrong.
|
My apologies the whole thing is confusing. I just sent you a member request for the Stan dev team which should give you sufficient privileges (check your email). If after clicking the email you still cannot merge the PR then I'll merge it. I find this really weird though and want to figure out why this is happening. I really think it's important and satisfying to let collaborators merge their own pull requests (first PR is a big deal!) |
Sorry, something went wrong.
|
@serban-nicusor-toptal do you know how to make it so outside collaborators can still click the merge button on their own PRs? |
Sorry, something went wrong.
|
Good morning @SteveBronder @Franzi2114, I think everything is setup correctly. In order for Franzi to get proper access, he will need to accept the invitation to join the Math org team (this team is mapped in the branch protections). Once that's done, the team will grant you the required permissions. |
Sorry, something went wrong.
|
YEEEEEEEEEEYYYYYYYYYY!!!!!!!!!! It worked!!!!!!!!!!!!!!!!!!! |
Sorry, something went wrong.
|
Wuhu! Thanks =D Thank you so much for all your proof reading, input, corrections and all the time you spent on this PR! I already prepared the CDF functions for the 7-parameter diffusion model. I will make the PR in the next days. |
Sorry, something went wrong.
|
Congrats on the huge effort! We can expose the basic scalar version in the compiler very soon (most likely next week!). A more advanced typechecking which lets us expose the vectorized version is something we’ve started discussing as well |
Sorry, something went wrong.
|
Congrats! Looking forward to testing this out and seeing the cdfs. Are you planning to implement the rng functions too? |
Sorry, something went wrong.
First, I do the CDF and CCDF functions. But compared to the CDF and the PDF, the rng should be quickly done. So I'll probably implement this as well. |
Sorry, something went wrong.
|
In terms of use I think the rngs would be really nice so folks can do prediction etc. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
We provide an implementation of the LPDF drift diffusion model with up to 7 parameters. There will be two new functions available in the stan::math namespace: wiener_full_lpdf and wiener_full_prec_lpdf. The only difference between both functions is the control of the precision in the computation of the partial derivatives. We did not deal with the CDF functions. With the new funtion it is possible to compute the 4-, 5-, 6-, 7- parameter drift diffusion models. Therefore, it is also an alternative to the wiener_lpdf by @kendalfoster.
The PR consists of the following new files:
Main file:
-stan/math/prim/prob/wiener_full_lpdf.hpp.
Helper files:
-stan/math/prim/fun/wiener5_lpdf.hpp: log density and partial derivatives for 5 parameter model
-stan/math/prim/fun/wiener7_lpdf.hpp: log density and partial derivatives for 7 parameter model
-stan/math/prim/functor/hcubature.hpp: new multidimensional integrator, adapted from Steven Johnson
-test/unit/math/prim/prob/wiener_full_test.cpp: own unit tests
-test/unit/math/prim/prob/wiener_full_prec_test.cpp: own unit tests for function with precision control
The following files are adapted in order to be able to implement a function with 8 parameters:
-stan/math/prim/functor/operands_and_partials.hpp
-stan/math/rev/functor/operands_and_partials.hpp
Discourse threads:
Testing for 7-parameter function with known partials -> Therefore, we only let scalar input be accepted and no vectorization.
Is there a multidimensional integrate-function? -> Therefore, we implemented hcubature.
How to document correctly?/How to add a new function’s documentation to the Stan Function Reference? -> Up to now, we have the doxygen documentation.
Adding a function with LARGE known partial derivatives -> We introduce 3 helper files.
HMC: Does precision of derivatives affect validity of results?
Issue:
Extension operands_and_partials to 8 edges #2698
Tests
stan/math/test/unit/math/prim/prob/wiener_full_test.cpp and wiener_full_prec_test.cpp
In this discourse thread we asked on how to test the new function. Since we have more parameters than all other functions, we wrote own unit tests to test all exceptions for invalid input, and tested whether valid scalar types give the correct result. Results are compared with values from the R-package WienR.
Side Effects
We had to adapt the operands_and_partials routine such that it accepts 8 parameters.
We had to implement a new, multi-dimensional integrator: hcubature.
Release notes
Two new functions in the stan::math namespace availabe: wiener_full_lpdf and wiener_full_prec_lpdf.
Along with some helper functions in stan::math::internal.
New integrator: hcubature.
Adaptation of operands_and_partials to 8 parameters.
Checklist
Math issue Adding 7-Parameter Drift Diffusion Model (DDM) PDF with partial derivatives to Stan Math #2682
Copyright holder:
- Franziska Henrich (franziska.henrich@psychologie.uni-freiburg.de)
- Valentin Pratz (pratz@stud.uni-heidelberg.de)
- Christoph Klauer (christoph.klauer@psychologie.uni-freiburg.de)
- Raphael Hartmann (raphael.hartmann@staff.uni-marburg.de)
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested