ICB-DCM / pyPESTO

python Parameter EStimation TOolbox
https://pypesto.readthedocs.io
BSD 3-Clause "New" or "Revised" License
206 stars 44 forks source link

NegLogParameterPriors evaluates prior density also on fixed parameters #1412

Closed arrjon closed 3 weeks ago

arrjon commented 4 weeks ago

Bug description NegLogParameterPriors evaluates prior density also on fixed parameters. This should not be the case and might be important in the case of model selection. Currently, this objective is not aware of free and fixed parameters and just evals the prior density for all parameters. In the case, where the nominal value might lie outside of the prior range, this will return always -inf, otherwise this will not raise any errors or warnings.

Expected behavior NegLogParameterPriors should only evaluate the prior on free parameters.

To reproduce Define a petab problem with fixed parameters, where these fixed parameters also have a prior defined.

Possible Solutions

FFroehlich commented 3 weeks ago

I have to disagree here. If you don't want a prior for a fixed parameter, don't specify one in the parameter table. In other words, in which situations would it be important to evaluate the prior for a fixed parameter?

In general, model selection metrics like AIC and BIC are only applied to the likelihood function, not the posterior and it feels more natural to use sampling based approaches with posteriors.

arrjon commented 3 weeks ago

I have to disagree here. If you don't want a prior for a fixed parameter, don't specify one in the parameter table. In other words, in which situations would it be important to evaluate the prior for a fixed parameter?

I agree that one should not specify priors for fixed parameters from the beginning. However, you can fix and unfix parameters later, and currently this will be ignored. For example, this can happen when you do model selection, where you fix parameters which were free in another model.

In general, model selection metrics like AIC and BIC are only applied to the likelihood function, not the posterior and it feels more natural to use sampling based approaches with posteriors.

True, however, if you compute Bayes factors, then the prior matters and also the value of the prior density.

FFroehlich commented 3 weeks ago

I have to disagree here. If you don't want a prior for a fixed parameter, don't specify one in the parameter table. In other words, in which situations would it be important to evaluate the prior for a fixed parameter?

I agree that one should not specify priors for fixed parameters from the beginning. However, you can fix and unfix parameters later, and currently this will be ignored. For example, this can happen when you do model selection, where you fix parameters which were free in another model.

But you could also argue that one can add and remove priors later. From my perspective the user can already specify whether there is a prior on a fixed parameter or not within the context of a petab problem.

Implementing that the prior is never evaluated for fixed parameter reduced the expressivity of the format. One could then of course introduce additional options to switch between behaviours, but that would then give us the same expressivity at the cost of a more complex syntax. Taken together, I don't see any net benefit, except that fixing parameters + removing priors is a bit simpler (which could also be implemented through functions in the petab library) at the cost of more complexity in every other situation.

JanHasenauer commented 3 weeks ago

I agree with Fabian.

For model selection it would be best to distinguish likelihood and prior contribution. Indeed, for a proper analysis this is necessary as the evaluation has to be performed at the maximum likelihood estimate. Accordingly, optimisation for model selection has to run without the prior.

arrjon commented 3 weeks ago

Okay, I also agree with your point, @FFroehlich.

It might be beneficial to include a warning when loading the PEtab problem if priors are specified for fixed parameters. This way, we retain the flexibility to add and remove them as needed while ensuring users are aware of potential unintended specifications and catch problems early on.

FFroehlich commented 3 weeks ago

Okay, I also agree with your point, @FFroehlich.

It might be beneficial to include a warning when loading the PEtab problem if priors are specified for fixed parameters. This way, we retain the flexibility to add and remove them as needed while ensuring users are aware of potential unintended specifications and catch problems early on.

Yes, that sounds like a reasonable approach.