Skip to content
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

harmonize LBA + fix bug #108

Merged
merged 1 commit into from
Dec 18, 2024
Merged

harmonize LBA + fix bug #108

merged 1 commit into from
Dec 18, 2024

Conversation

Zooaal
Copy link
Contributor

@Zooaal Zooaal commented Dec 18, 2024

We noticed a interface-difference and two bugs in LBA.jl:

  • instead of specifing Δt like in other functions (e.g. DDM), one specifies the number of timesteps.

  • But this number was not actually used to evaluate the evidence, thus resulting in a missmatch between the timesteps and the evidence.

  • one rand function didnt use the rng-object

This might be judged as breaking because of interface change, maybe a wrapper is possible? But at least the bug should be fixed in anycase.

Best, Timo & @behinger

PS: Note that we havent tested the fix yet locally🙈

@kiante-fernandez
Copy link
Contributor

Thanks for the catch. @itsdfish might make sense to just swap order of time_steps and evidence here such that:

function simulate(rng::AbstractRNG, model::AbstractLBA; Δt = 0.01, _...)
    (; τ, A, k, ν, σ) = model
    b = A + k
    n = length(ν)
    νs = sample_drift_rates(rng, ν, σ)
    a = rand(rng, Uniform(0, A), n)
    dt = @. (b - a) / νs
    choice, t = select_winner(dt)
    time_steps = range(0, t, step = Δt)  # Define time steps first with Δt
    evidence = collect.(range.(a, a + νs * t, length = length(time_steps)))  # Match evidence to time steps
    return time_steps, hcat(evidence...)
end

Also from my memory this will also apply to the LNR.jl implementation.

@itsdfish
Copy link
Owner

@Zooaal, thank you for bring this to my attention. At point, I was aware of the inconsistency in the interface, but forgot to fix it. I think there are different ways to handle the versioning, but to be on the safe side, I classify it as a breaking change even though it is an error. Good catch regarding rng.

@kiante-fernandez, I will also fix LNR and fix the tests. I also noticed that the variable choice is not used, so I will change it to _. I will release a new version by tomorrow morning. Thanks!

@itsdfish itsdfish merged commit 1881223 into itsdfish:master Dec 18, 2024
1 of 4 checks passed
@itsdfish
Copy link
Owner

I did not know it was going to automatically merge when I pushed a change to the PR. Weird.

Anyways, I noticed the Zooaal set the delta_t to .01, but the other functions used a default of .001. Is there a strong preference for .01? If so, I can change it to .01 since I am already making a breaking change.

@Zooaal
Copy link
Contributor Author

Zooaal commented Dec 18, 2024

First of all thank you for the reaction.

delta_t to .01 was a mistake on my part, because I wanted the same as for the DDM .001. Sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants