-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add stDDM to SequentialSamplingModels #59
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 89.97% 90.20% +0.22%
==========================================
Files 23 24 +1
Lines 1417 1480 +63
==========================================
+ Hits 1275 1335 +60
- Misses 142 145 +3 ☔ View full report in Codecov by Sentry. |
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Sounds like this will be a nice addition. Thanks! |
I have not read the paper Amount and time exert independent influences on intertemporal choice in detail, but I am wondering whether this model should be a subtype of |
…etween drift rates
@itsdfish I added some docs and tests, along with making changes throughout. |
@kiante-fernandez, awesome! I look forward to reviewing the new model. I will review piecemeal over the week and will ping you when I am done. |
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.
@kiante-fernandez, thanks again for the PR. Aside from the minor noted issues, I think it looks great. Once those are addressed, I will merge. I did not test the docs locally. Please do to ensure they work if you have not done so already. Thanks!
using Random | ||
Random.seed!(8414) | ||
|
||
parms = (ν = [1.0, 1.2], α = 0.8, τ = 0.30 , s = 0.0, z = 0.50) |
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.
This looks good. Would you be able to add a second test where you change each parameter just to be safe? At the very least, it would be good to test that the results agree with s > 0.
|
||
dist = stDDM(; α, ν = [-3, -3], Δt) | ||
time_steps, evidence = simulate(dist) | ||
@test evidence[end] ≈ 0.0 atol = 0.05 |
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.
This first set of tests make sense to me, but I'm not sure why evidence[end] ≈ 0.0
in the second test.
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.
simulate
can hit (x < α) && (x > 0). It is the corresponding other bound. We would expect that two large negative weights on drift would hit the lower bound.
@kiante-fernandez, thanks for the PR! |
Out of curiosity, how does stDDM relate to Ratcliff's DDM? Can the two be merged/combined in principle? or are they inherently distinct? |
The article below explains the differences between the two models. From what I have gathered, the DDM is a special case of the stDDM in which the starting time of processing different attributes occurs at the same time. Piecewise constant averaging methods allow for |
Which means that cross-trial variability (that #29 implements) could be in-principle added? Or am I getting confused? |
I am fairly certain it is possible to compute the pdf for this model according to equation 9 in the paper. The only thing I am uncertain about is the correlation between the drift rate components here. I think I am going to change the name. Evidence doesn't seem the most descriptive. |
Iinked to dicssion in #41
Since the model sees extensive use, I figured an accessible simulator could be useful. I still need to write the doc page and test for the model but here is the PR to get us started.