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

Non-integer parameter sample from discrete prior #30

Open
mauricelanghinrichs opened this issue Jun 3, 2021 · 3 comments
Open

Non-integer parameter sample from discrete prior #30

mauricelanghinrichs opened this issue Jun 3, 2021 · 3 comments

Comments

@mauricelanghinrichs
Copy link

Hey!

first, thanks a lot for this package! I was particular happy to see the support for (mixed continuous/) discrete priors, and the smc method seems to work well in my initial runs so far. However, I noticed a potential bug when using the ABCDE method (not yet released, current master branch of KissABC v3.0.1). When using a discrete prior the parameters from that prior are (sometimes) non-integer values (in contrast to smc).

Would be awesome if someone could take a look, as I'm excited to try out also the DE method!
The following code produces the behaviour for me (Julia 1.6, macOS, Distributions v0.23.12):

using KissABC
using Distributions
using Random

Random.seed!(1)
const p1 = 80.0
const p2 = 10.0
const targetdata = rand(Normal(p1, p2), 1000)

function dist((μ,σ))
    # isinteger(μ) && @info("Indeed integer ", μ)
    # isinteger(μ) || @warn("Found non-integer ", μ)

    # Int() will throw error
    simdata = rand(Normal(Int(μ),σ), 1000)
    d1 = mean(simdata) - mean(targetdata)
    d2 = std(simdata) - std(targetdata)
    hypot(d1, d2)
end

# mixed discrete/continuous prior
const prior=Factored(DiscreteUniform(0.0, 200.0), Uniform(0.0, 50.0))

# smc runs fine
const res_smc = smc(prior, dist, verbose=true)

# ABCDE throws error like InexactError: Int64(22.32...)
const res_DE = ABCDE(prior, dist, 0.3, nparticles=100, generations=100)

Best

@pcjentsch
Copy link

I am not the developer of this package, but I have been using it for the past year or so. The ABCDE method was in v1 of this package but was removed because the posteriors it produces have "bias" (see this discourse post by the author). They mention that this bias can be overcome by running it for additional iterations.

The author added the method back, undocumented, after I pointed out that it performs far better on some of the big disease model problems I work on.

I haven't used it with mixed priors, so I cannot comment on your problem, I think it's probably a misplaced promotion thing. I hope this context was helpful though! If you know more about the differential evolution method and it's implementation then I certainly would find it really helpful if you know about the aforementioned problem.

@mauricelanghinrichs
Copy link
Author

@pcjentsch thanks for your comment! This bias was only mentioned for earlystop=true, right? I think default is false and I hope that is fine to use (?). The DE method seems to be also faster for my problem, so I would be quite happy to (safely) continue using it.. :D

@francescoalemanno
Copy link
Owner

Thank you @pcjentsch and thank you @mauricelanghinrichs!
As soon as I get some free time I would like to fix the state of this package and make a good release,
However if you guys can get to it earlier I would very gladly accept a PR...
Very sorry about the issues

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

No branches or pull requests

3 participants