EpiAware / PrimaryCensored.jl

Primary censoring extension for Distributions.jl
http://primarycensored.epiaware.org/
8 stars 1 forks source link

Add a solver arg for `cdf` function #34

Open SamuelBrand1 opened 2 weeks ago

SamuelBrand1 commented 2 weeks ago

Problem

A nice feature of using Integrals.jl compared to QuadGK.jl is that we have a common interface to multiple different quadrature methods. But at the moment we can't use them because the equivalent method to QuadGK is hard coded into cdf call.

Possible solution

Pass a kwarg to cdf which defines a quadrature solver, e.g

function Distributions.cdf(d::PrimaryCensoredDist, x::Real; solver = QuadGKJL())
... some code ...
    result = solve(prob, solver)[1]
    return result
end

Alternative solution

No change or encode solver as part of the PrimaryCensoredDist struct.

seabbs commented 2 weeks ago

Yes agree but should this be arg to the struct or to the cdf? The benefit of the struct is we can inject changes inside nested code. The advantage of the cdf is you can make different choices in different places easily and its closer to the use case

*Edit: as I see you flag in the alternative suggestion. I think my preference is the struct in order to allow for changes inside a complex use case without moving the arg around but perhaps that is the wrong shout)

SamuelBrand1 commented 2 weeks ago

Yes agree but should this be arg to the struct or to the cdf? The benefit of the struct is we can inject changes inside nested code. The advantage of the cdf is you can make different choices in different places easily and its closer to the use case

*Edit: as I see you flag in the alternative suggestion. I think my preference is the struct in order to allow for changes inside a complex use case without moving the arg around but perhaps that is the wrong shout)

I've come around to second approach too

seabbs commented 2 weeks ago

The argument being that PrimaryCensored defines the complexity of the problem and so it makes sense you would want to tie a specific solver to that problem across all use cases

SamuelBrand1 commented 2 weeks ago

Yes that makes sense to me. Its more similar to Distributions.jl where check_args gets passed at construction rather than being a kwarg to (say) logpdf