sebapersson / PEtab.jl

Create parameter estimation problems for ODE models
https://sebapersson.github.io/PEtab.jl/stable/
MIT License
29 stars 4 forks source link

Function `get_ps` possibly unexpected behaviours #157

Open TorkelE opened 7 months ago

TorkelE commented 7 months ago

I noted two things about this function:

1) If PEtab add initial condition parameters, these are added here (probably related to https://github.com/sebapersson/PEtab.jl/issues/155). This probably should not be the case, and this should be provided with the corresponding initial condition function instead.

2) If there are fitting parameters that are not part of the ReactionSystem, these are not provided with get_ps. I.e. if I declare a noise function for the data, create a PEtabParameter of it, and fit that as well. Don't think anything is broken here, but figured it should be noted (which it kind of is already). Maybe one should consider a function for getting a full parameter set (just because it can be interesting to check.

(These points probably are rather specific to when ReactionSystems are created directly, instead of loaded from files)

sebapersson commented 7 months ago

Thanks for noticing this.

  1. I agree, and this is one of the things that should be fixed with #155
  2. This is an interesting note. Because you can get the parameters via res.xmin, however the parameters are on log-scale (by default), which seems to confuse a lot of users. So how about a function that returns on linear-scale, and that results can be returned as a vector or a map?
TorkelE commented 7 months ago

Yes. Working on some stuff using PEtab (but combining with e.g. https://github.com/insysbio/LikelihoodProfiler.jl) and it is non-trivial to keep track of what is log and lin scale.

Would it make sense to create accessor functions for most fields, and then not document them officially for the users? Then functions which returns parameters values could have a kwarg for the scale.

Then you can have

get_model_ps # Returns parameters required for simulation.
get_fitting_ps # Returns parameters that are not part of the model.
get_all_ps # Returns all parameters.

(or maybe let the first one have its current argument)

For the objective function, it would also be useful to be able to access it both on log and linear scale.

For me, right now, because PEtab handles Linear -> Log conversion, I tried to keep everything linear outside of PEtab. But then For e.g. the objective function (or xmin) I can only get stuff in log scale, which means I need to convert my non-PEtab stuff to log-scale (but keep them linear scale before giving to PEtab).

Basically, it would be good with some way to easier keep track of when stuff is in log and linear scale.

sebapersson commented 7 months ago

I think you mean PEtab handles Log -> Linear scale internally.

But I agree on the note with util function (and not really having the user access the PEtabODEProblem fields at all unless they know what they are doing). So how about something like:

get_model_ps # Returns parameters required for simulation.
get_fitting_ps # Returns parameters that are not part of the model (called non dynamic)
get_all_ps # Returns all parameters.
get_ps_names # Get the names, all should have corresponding
to_estimation_scale # For transforming to linear to parameter estimation scale

For having an objective function on both log and linear scale, I am a bit hesitant about this, as the user can easily mess up. For the objective function it does not matter, but for the gradient scale matters;

petab_prob.compute_gradient_lin(p) != petab_prob.compute_gradient_log(log.(p))

and I am afraid that if we have both, and a user would like to for example wrap any functionality we do not currently have they might end up using the linear version and get suboptimal performance (any autodiff on the objective function will also be suboptimal).

What do you think?

TorkelE commented 7 months ago

On a second thought I agree about the objective function. Ideally, eventually, things will be integrated enough across the ecosystem that the user wonät have to extract these themselves for e.g. likelihood profiling.

Otherwise it sounds good. Still think the estimation scale would be a good kwarg to the other functions, but it is a small thing.

At some point one might consider similar accessors for thee remaining fields of interest of the optimisation problems, but might be more work than it is gain.