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

Add ability to extract model parameters for simulation #88

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

sebapersson
Copy link
Owner

See #85

The new function can cover pre-eq (by unfortunately having to rerun the pre-eq simulation but there is no way around), and model with condition specific parameters. Moreover, either a map or vector can be returned via retmap.

The function is tested.

I am struggling a bit with finding a good name. Feedback appreciated @TorkelE.

When all looks good I can improve docstring and merge with main.

@TorkelE
Copy link
Contributor

TorkelE commented Sep 28, 2023

Maybe something like get_model_fit or get fitted_model. get_fitted_u0_ps is descriptive (but not very pretty)

Would it be possible to make two sub functions, appending _ps and _u0 (extracting just those parts of the fit.

@sebapersson
Copy link
Owner Author

You are thinking two functions:

get_fitted_ps
get_fitted_u0

I like this solution the best as what the functions do is clear, and then the user will not have to confused whether the function returns u0 or p as first argument. It will also be easy to code.

If you do not have any objection I will go with this.

We can probably also provide the user with a function named get_fitted_ode (or similar) returning an ODE problem with ps and u0 correctly placed.

@TorkelE
Copy link
Contributor

TorkelE commented Sep 28, 2023

Sounds good. Actually, the get_fitted_ode returning the full ODE Problem sounds like a really good idea. That should totally get implemented at some point.

@sebapersson
Copy link
Owner Author

I splited into two functions, and added docstrings.

If you think it looks good I will merge with main.

It will also now be easy to do get_fitted_ode (I think), as it should simply be:

function get_fitted_ode(res::Union{PEtabOptimisationResult, PEtabMultistartOptimisationResult}, 
                                      petab_problem::PEtabODEProblem,
                                      condition_id::Union{String, Symbol}; 
                                      pre_eq_id::Union{String, Symbol, Nothing}=nothing)::ODEProblem

    u0, p = _get_fitted_parameters(res, petab_problem, condition_id, pre_eq_id, retmap)
    return remake(petab_problem.ode_problem, p=p, u0=u0)
end

@TorkelE
Copy link
Contributor

TorkelE commented Sep 28, 2023

Looks good.

Maybe, given that condition_id can be optional (if only 1 condition is used), I'd consider making it an arg, with a default value equal to the default condition name.

@sebapersson
Copy link
Owner Author

I agree, I have now added this. To also handle models with a single specified simulation condition if condition_id=nothing it now defaults to the first simulation condition.

@sebapersson sebapersson merged commit 31297df into main Sep 29, 2023
3 of 4 checks passed
@sebapersson sebapersson deleted the Extract_model_parameters branch September 29, 2023 06:29
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.

2 participants