search

ICB-DCM / pyPESTO

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

importer.create_objective() should return the negative log posterior or just the neg. log likelihood ? #594

Open elbaraim opened 3 years ago

elbaraim commented 3 years ago

I am using a model containing PEtab priors. Therefore my problem.objective is an aggregated objective containing negative log likelihood + negative log prior. At the moment, calling importer.create_objective() returns the negative log likelihood only. But to my mind, it would make more sense that importer.create_objective() returns the aggregated objective (including the prior), to have it consistent with the problem.objective. Otherwise maybe consider renaming the method to create_negloglikelihood or something like this (?).

I just wanted to open a small discussion on this to hear your thoughts.

FFroehlich commented 3 years ago

Agreed

elbaraim commented 3 years ago

Another thing/issue related to this question: When setting, e.g., different AMICI solver options one needs to do that on the importer.create_objective() object since there is not amici_solver field in the obtained aggregated objective problem.objective (in my case).

For example, I want to set some solver options such that then my simulation and gradient evaluation is successful. Then when calling problem.objective the error still persists, while when calling the objective from importer.create_objective() it is successful. Problem here is that the objective from importer.create_objective() in my case is only the negative log likelihood, but not the negative log posterior. So, if I want to set some custom settings on the solver, seems that I cannot use anymore the aggregated objective that was directly created. So, I need to create a new problem again with the objective with the updated settings + the prior ...

EDIT: or maybe I did something wrong?

yannikschaelte commented 3 years ago

Regarding the last point, one can always pass solver objects to create_problem too, that would be the cleanest way.

Regarding llh vs lpost: I have the impression that the posterior vs likelihood usage is not very clean yet in pypesto. In most parts, we want the lpost (Bayesian analysis), therefore I would suggest to by default always include the lprior, but add flags indicating whether only the llh should be used. Aliases create_llh, create_lpost would make sense in addition.

FFroehlich commented 3 years ago

For example, I want to set some solver options such that then my simulation and gradient evaluation is successful. Then when calling problem.objective the error still persists, while when calling the objective from importer.create_objective() it is successful. Problem here is that the objective from importer.create_objective() in my case is only the negative log likelihood, but not the negative log posterior. So, if I want to set some custom settings on the solver, seems that I cannot use anymore the aggregated objective that was directly created. So, I need to create a new problem again with the objective with the updated settings + the prior ...

EDIT: or maybe I did something wrong?

I agree this is an issue and it would be nice to be able to pipe arguments to model/solver through the aggregated objective.