Repository navigation
Review for SequentialSamplingModels.jl #126
Description
Hi all!
I'm one of your reviewers for the The Proceedings of the JuliaCon Conferences. Overall, I think the package is a great addition to the ecosystem, but I have comments on the following items that I would like to hear your rebuttal or see changes before signing off.
Comments About the Code
- The does not have error handling of invalid parameter combinations. For example, if I specified the following DDM model:
DDM(ν = 1.0, α = -0.8, z = -473.89, τ = -1.0)
no error is thrown. I think using either ArgCheck.jl - Many of the simulate functions could benefit from better memory allocation approaches. One possibility is to use something like
sizehint! - More of a personal matter, but i think restructuring the src directory to have subdirs like you do on the documentation would be great.
- Using specific seeds for the Random module is discouraged. 1.) Using randomness can make tests more reliable 2.) If you want to use RNGs, use StableRNGs
- Personal choice, but I would suggest making the testing folder its own julia "project" with its own Project.toml file. I think this will make things easier to debug and you can remove all of the repeated library imports.
- Instead of overloading Random.rand, you could define a custom sampler for your models which could simplify the API for some of the models.
Something like this perhaps (I wrote this quick so might need to iron out some details...):
# Abstract type for all samplers
abstract type AbstractSSMSampler end
# Generic sampler struct that holds model + auxiliary data
struct SSMSampler{M<:Union{SSM1D,SSM2D}, D} <: AbstractSSMSampler
model::M
data::D
end
# Single rand method that dispatches on sampler type
function rand(rng::AbstractRNG, s::SSMSampler)
_rand(rng, s.model, s.data)
end
- Some of the ways "invalid" RTs are handled is inconsistent. For example, in the DDM:
if τ ≥ t
return T(NaN)
end
but in LBA:
rt < τ ? (return 1e-10) : nothing
I think choosing a small positive constant makes more sense, and I would make this consistent across the package. Unless there is a reason I am missing?
Documentation
Overall very few comments.
- I would make sure you have a stable documentation not just a dev documentation.
- I feel the contribution guidelines are a little light. I think it would also be great if they were more front and center? Maybe on the home page?
- I would turn your examples in the docstrings to doctests.
- Would you consider adding codecov? It would be nice to know how much code is tested.
Comments on the paper
- There are some plot rendering issues on the plots. For example, there are overlapping labels in figure 4.
- Mentioning again here, you link to the dev documentation. I would change this to stable.
Activity
rsenne commentedon Jul 24, 2025
Accidentally hit enter before finishing gonna continue editing :)
kiante-fernandez commentedon Jul 25, 2025
Thanks @rsenne ! Can you post your final review here: JuliaCon/proceedings-review#186 (comment)
That way we have both reviews in the same place and the editor can more easily keep track of them.
itsdfish commentedon Jul 25, 2025
@rsenne, thank you for your feedback. I think we can make most of these changes without too much effort and it would improve the code base.
@kiante-fernandez here is a table of the issues. I assigned myself to a few of the issues. Please coordinate with me on any issues you would like to fix.
sizehint!seems to make performance slower. Marked as complete unless others identify other performance improvements.@argcheckis the best option because it has an informative error message, but does not have a noticeable performance penalty. I will commit a fix this week.\bibliographystyle{authordate1}to\bibliographystyle{plainnat}. I added missing DOIs.rsenne commentedon Jul 31, 2025
Another question I realize i forgot to ask: how do you feel about extending some of these models to have distributions over parameters? I feel some of these models are only used when that is the case (the most prominent being the Extended DDM ala the Ratcliff homogeneous noise process)?
Also, it looks like you had a few issues with my suggestions. I'll do my best to provide help on these points:
Did you set up a
CODECOV_TOKEN? See my CI here.It doesn't look like you have the right permissions set up in your CI.yml file. I recommend using the default one supplied in the link.
itsdfish commentedon Jul 31, 2025
Thank you. I did not realize the token was missing. The codecov docs pointed me in the wrong direction. Right now it reports 88% coverage. I'll gradually increase the coverage.
I'll see if I can fix the permissions for stable documentation>
Good question. Kiante started working on adding inter-trial variability in drift rate and non-decision time to the DDM, but run into some issues. I haven't had time to debug, but it will be a good addition to the package.
rsenne commentedon Oct 16, 2025
Just wanted to check in on this. Hope the reviews are going well :)
itsdfish commentedon Oct 19, 2025
@rsenne thanks for the follow up. I think I was able to address most of the comments. Is there any action I need to take in order to proceed?
rsenne commentedon Oct 20, 2025
Hi @itsdfish . Could you provide a quick write-up of any changes you couldn't make and just give me a quick 1-2 sentence blurb on why they weren't accomplished (Feel free to just copy past anything you've already written in the table above and then whatever else)? Overall, I think you've done a phenomenal job addressing my comments so I think is will be pretty quick. I just to keep mental tabs on what has been done and not.
I also want to check back on my other suggestion of making this work with distributions over the parameters? This seems pretty central to many aspects of these models (at least in my experience with variants of the DDM), maybe just an example in Turing how one could accomplish this (does this exist already)? Or at the least a discussion/open issue for extending the current model classes to support this. I myself, before I knew of the existence of this package, have written my own WFPT code and used FastGaussQuadrature.jl to integrate out the nuisance variables and get good model fits. I myself could even do this if you were interested.
After all this, I will let the editor know my review is complete. Then you will just have to address all the other comments from @ZachLoschin
itsdfish commentedon Oct 20, 2025
@rsenne, the main item I did not address was this:
Instead of overloading Random.rand, you could define a custom sampler for your models which could simplify the API for some of the models.
If I understood correctly, this change would only provide a small benefit and would not change the user experience.
Regarding your second question, the LBA, RDM, and Wald models have distributions over some parameters which are marginalized out analytically. Kiante has an PR in which he implementing the full DDM with a uniform distribution over non-decision time and a normal distribution for the drift rate across trials. Kiante made good progress, but ran into some issues. I haven't had time to inspect the code in detail. Your idea with FastGaussQuadrature.jl sounds interesting. At some point, we tried QuadGK.jl, but it didn't work well with automatic differentiation (AD). The problem is that AD propagated through the integration routine and slowed things down to a crawl. Perhaps there is a custom rule that can be written to prevent AD from propagating through the numeric integrator, but AD is a bit outside of my expertise. We would definitely appreciate a PR to the full DDM working. Before getting into the details, I recommend creating a simple model to test whether FastGaussQuadrature.jl plays well with ForwardDiff and Mooncake ADs.
This weekend, I will have time to address comments from ZachLoschin. I will incorporate them into the table above and ping you when it is complete. Thanks again!
itsdfish commentedon Oct 24, 2025
@kiante-fernandez, I was wondering whether you can address the final issue below? Thanks!
To be consistent with your example, perhaps the figure one labels can be changed from "Choose left" and "Choose right" to "Choose Margherita" and "Choose pineapple".
Update
I was able to change the labels without a noticeable decrease in image quality. I was concerned that not having the original files would have resulted in loss in quality.
itsdfish commentedon Oct 26, 2025
@rsenne, I was able to address the remaining issues including the labeling of Figure 1. Please let me know if I further action is needed to proceed.
rsenne commentedon Oct 27, 2025
I would revive the review thread and let the editor know that you are done with the reviews. You'll need a sign off from myself and @ZachLoschin . Should be fairly quick after that. I will probably try getting a working eDDM this week and see if at the least I can get a working example--though I won't let this stand in the way of publishing.
itsdfish commentedon Dec 14, 2025
https://zenodo.org/records/17932370?token=eyJhbGciOiJIUzUxMiJ9.eyJpZCI6IjkzZjFlODJkLWZkMzItNDJhYi04NzlkLTkzODIzNzg0MjZjYyIsImRhdGEiOnt9LCJyYW5kb20iOiJlYTAyNzEyM2M3ZDRlYWIzOTc1MWQ4OGQ3NzJkMmJjYyJ9.nZjcH-dLv-u34WHWhIGBJNZSXNgZg5D7sRkkSWBHY1Zmh3qq2DHaa7_-UPYOGlfidbCPtFfBxIqnwWeSllHvcA