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 does not evaluate prior density at fixed params #1413

Closed arrjon closed 3 weeks ago

arrjon commented 4 weeks ago

Based on #1412

Making the NegLogParameterPriors aware of fixed parameters. The density is not evaluted for those. Moreover, the petab importet is updated to indicate the fixed parameters while creating the prior.

Question: should this argument stay optional? If priors are not given for fixed parameters, no problem would occure. The problem only occurs when priors are given and parameters fixed.

codecov-commenter commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.90%. Comparing base (2250dbc) to head (b178863).

Files Patch % Lines
.../objective/roadrunner/petab_importer_roadrunner.py 50.00% 2 Missing :warning:
pypesto/petab/importer.py 50.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1413 +/- ## =========================================== - Coverage 83.92% 83.90% -0.03% =========================================== Files 160 160 Lines 13061 13067 +6 =========================================== + Hits 10962 10964 +2 - Misses 2099 2103 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dilpath commented 3 weeks ago

Thanks! Sorry for the mask suggestion, I wasn't familiar with the code

arrjon commented 3 weeks ago

Also not entirely sure whether it is relevant, but the problem has a function fix_parameters, for this it should be accounted as well? 🤔

That is relevant, but the function calls normalize, which in turn calls update_from_problem, so no need to add in fix_parameters.

arrjon commented 3 weeks ago

After discussing this issue in #1412, we will not make the prior aware of fixed parameters. Instead, users should specifiy priors correctly in the PEtab problem. In this way, we will allow maximum flexibility.