-
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
WIP: Ratcliff Diffusion Model pdf and simulators #29
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
===========================================
- Coverage 82.93% 52.80% -30.14%
===========================================
Files 11 12 +1
Lines 545 856 +311
===========================================
Hits 452 452
- Misses 93 404 +311
☔ View full report in Codecov by Sentry. |
src/RatcliffDDM.jl
Outdated
end | ||
|
||
if η == 0 |
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.
I like the idea of suggesting typical parameter ranges in the documentation, but I don't think we should enforce bounds unless they are strict (e.g., a standard deviation must be non-negative). I think the best approach is to allow users to enforce their own parameter bounds. This will also be consistent with the approach taken for the other models in the package. .
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.
Something that might be good to add is argument checks for parameters that cannot take certain values (e.g., negative values). Here is the best approach, which could be used throughout the package:
struct M{T<:Real}
x::T
y::T
function M(x::T, y::T; check=true) where {T}
check ? check_args(M, x, y) : nothing
return new{T}(x, y)
end
end
function M(x, y; check=true)
return M(promote(x, y)...; check)
end
function M(; x, y, check=true)
return M(x, y; check)
end
function check_args(m::Type{<:M}, x, y)
if (x < 0) || (y < 0)
error("x and y must be positive")
end
end
m = M(1, 2)
m = M(; y=1, x=2)
m = M(; y=1, x=-2, check=false)
m = M(3.0, 1)
m = M(-2, 1)
m = M(-2, 1.0)
The inner constructor is kind of ugly, but it gives us a lot of flexibility.
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.
Okay. I will make a separate PR that contains an implementation for this across models.
@kiante-fernandez, thanks for the progress update. I realize this is a WIP, but I left a few comments that may still be applicable. My approach to tests is to start by plotting the pdf over the histogram of simulated data. That is part of the reason I include those plots in the documentation. I also like to compare the pdf to a kernel density of the pdf. It's not perfect, but it gives you some idea of whether the generative model and the pdf are consistent with each other. I also test against established libraries when available. Another thing I do is develop a test whenever I find a bug to ensure there are no regressions in the future. |
Could we also add (in the docstrings?) a brief mention about the difference between DDM and RatcliffDDM and maybe a cross-link ("See also |
If we can get this model working, another option would be to integrate everything into one model given that |
@itsdfish I have added some updated PDF along with the CDF code, but I am getting a precompile error that might be related to some bugs with Bijectors. |
Thanks @kiante-fernandez for your hard work. It is difficult for me to look at a fork locally. Based on the unit tests, it seems like there is a variable called x that is not defined somewhere. It's not clear to me where its at based on the error trace. I recommend using Revise.jl and commenting out files or parts of files until you can locate the issue. If you continue to have issues, I can make a new branch and you can merge into that. I don't want to merge code with errors into main. I'll leave a few comments for now and look through the code in more detail probably tomorrow. |
@kiante-fernandez, I hope you are doing well. I wanted to follow up with you about the status of the PR and a related question about Julia
R
The response probabilities are similar, but the rts are different. Any ideas? |
Thanks for following up. With the holiday here, I should have some time to take a look. Also, from the example you gave, you have set z to two different values (Julia: z = 0.25 and R: z = 0.20). |
Thanks for taking a look. You are right that the values for Anyways, have a good break!
Update After further investigation, I found that the culprit is st0 and st. When I set the variability parameters to zero, the results agreed. However, the only diverged when st0 and st > 0:
The mean reaction time increased for rtdists, but stayed the same for SSM.jl. If I understand correctly, st0 should not change the mean reaction time. Is that your understanding too? |
@itsdfish That is my understanding as well. I tried to hunt down the sampling function
Here fast_dm is the
So at first glance, we might have found something wrong with Also, just so I am clear on the PR, I am still writing DDM.jl to replace RatcliffDDM.jl. So, when we have non-zero values of the across-trial parameters, it just calls those functions (the RatcliffDDM) instead? |
@kiante-fernandez, thanks for looking through the issue above. It does appear to be a bug. I will submit an issue later today or tomorrow. I think the code above can be repurposed for unit tests at some point. It would be good to include a few tests to make sure changes in parameters have the expected effect (or lack of effect). It goes to show that it is easy to make a simple error. I would be happy to add those tests if it would help. Yeah. I think it would be good to remove the DDM code and use the RatcliffDDM code because it subsumes the DDM code. There are probably different ways to approach it. Perhaps the simplest way is to use conditional checks to skip calculations needed for st, eta and sz if the value is equal to zero. For example, if you perform an expensive operation (e.g., a convolution) in the RatcliffDDM code, we can potentially skip it if eta = 0. Of course, you will probably have a better idea of what approach is the best in terms readability, efficiency, and maintainability. But I think this will be simpler overall because DDM is a special case where st, eta,sz = 0 in RatcliffDDM. Once all of that is done, I recommend switching the name from RatcliffDDM to DDM, which will prevent a breaking change (e.g., we are just making DDM more general). Let me know if you have any questions or concerns. Update I looked through the documentation for rtdists again and noticed I may have misunderstood the parameterization. Here is what it says "inter-trial-variability of non-decisional components. Range of a uniform distribution with mean t0 + st0/2 describing the distribution of actual t0 values across trials. Accounts for response times below t0." In contrast to sz, st0/2 is added to mean non-decision time. That seems a bit unusual to me, but is consistent with the observed behavior. The only thing that does not make sense to me is "Accounts for response times below t0". Maybe that is not important. Anyways, sorry for the oversight. I thought I read something else. |
@itsdfish I have finally had a chance to take a look through the new codebase; great work on organizing things! Following the conventions, I added the AbstractDDM and moved everything we needed to DDM.jl. I am still failing some of the tests I set up, but I thought I would ping for feedback while I have time to have eyes on this for a few days. |
@kiante-fernandez, thanks! I think we are converging on a good system and API. I made a few comments throughout. Everything looks good for the most part. I think the primary item is getting the unit tests to pass and adding more if needed. |
@itsdfish Thank you for the comments. I have been working to resolve some issues, but I have not yet identified what is wrong with the implementation. I thought it might be due to the numerical integration implementation (inspired by [https://github.com/hddm-devs/hddm/blob/master/src/integrate.pxi]), though I have not pinned it down yet. That is just my sense from some of the test results so far. |
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Should we aim at merging this before @kiante-fernandez's talk ^^ ? |
I'll close this now that it has been moved to #90 |
I have added a PDF file containing the Ratcliff Diffusion Model and two methods for simulating the diffusion process. However, the PDF code still needs testing as there are some remaining bugs. I wanted to share the progress so that we can identify the tests that need to be conducted and address any issues.