ProfileLikelihood.jl icon indicating copy to clipboard operation
ProfileLikelihood.jl copied to clipboard

Thoughts on 1.0

Open DanielVandH opened this issue 1 year ago • 8 comments

The package needs a bit of love to get it to 1.0. No guarantees on when all of the below can be accomplished, but it should be written down. In addition to the below, the code just needs to be simplified so much - I wrote this package right around when I first started getting serious about Julia, so there are some weird things in the code.

Simplifying `ProfileLikelihoodSolution

I think the result of profile should return some ProfileLikelihoodSolution (or whatever) type object (like there is now), but this object should not have so much contained in it. Instead, users can query it to compute e.g. confidence intervals. Currently, I have

Base.@kwdef struct ProfileLikelihoodSolution{I,V,LP,LS,Spl,CT,CF,OM}
    parameter_values::Dict{I,V}
    profile_values::Dict{I,V}
    likelihood_problem::LP
    likelihood_solution::LS
    splines::Dict{I,Spl}
    confidence_intervals::Dict{I,ConfidenceInterval{CT,CF}}
    other_mles::OM
end

I think we can remove splines and confidence_intervals, leaving

Base.@kwdef struct ProfileLikelihoodSolution{I,V,LP,LS,OM}
    parameter_values::Dict{I,V}
    profile_values::Dict{I,V}
    likelihood_problem::LP
    likelihood_solution::LS
    other_mles::OM
end

I think the types should also not be so generic, simplifying the design a bit. The confidence intervals and splines can then be computed using some other function like get_confidence_interval(sol, ...), which wouldn't be a breaking change necessarily since that is already the interface (which I have failed to properly define, though). Similarly for splines.

Simplifying individual steps

There should also be some Profiler struct (or some better name) which is used to take individual steps when profiling. This would make it easier to debug, and to also solve problems like #91 much easier. This would still be a bit restrictive though, since what if eventually we want to implement a method that returns only the confidence limits rather than the complete profile. I won't worry about this last point, though, since that's not on the horizon - let's worry about that when (or if) it gets there.

Simplifying the main functions mle and profile

profile takes quite a lot of keyword arguments. I think I can simplify it down a lot and just enforce some specific defaults. I worry it might be a bit overwhelming the way it is currently.

Simplifying how LikelihoodProblem is defined

Currently, LikelihoodProblems are constructed with an awkward mix of ProfileLikelihood.jl specific kwargs and Optimization.jl kwargs (via f_kwargs and prob_kwargs). Maybe I should just require that users define their own OptimizationProblem within Optimization.jl? Perhaps a LikelihoodFunction is needed so that I can use dispatch on OptimizationProblem to make the appropriate conversion.

Simplifying how likelihoods from ODEs are defined

The method for constructing a LikelihoodProblem for a differential equation is a bit wack. I designed it before I properly understood the SciML interface - I should just be implementing an init method or something.

DanielVandH avatar Dec 06 '23 05:12 DanielVandH