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

How to set `fill_fixed_parameters`=True for petab parameter mappings? #1448

Open FFroehlich opened 2 months ago

FFroehlich commented 2 months ago

So there is https://github.com/ICB-DCM/pyPESTO/blob/ead29b36db80d2eeee91b5d29bfbf4430137f8aa/pypesto/petab/importer.py#L476, which I don't like as this means that edata.plist will always be all parameters instead of subsetting to the ones that actually require sensitivities.

Now the best way that I found to fix this is

pypesto_problem = importer.create_problem()
simulation_conditions = petab.get_simulation_conditions(
    importer.petab_problem.measurement_df
)
pypesto_problem.objective.parameter_mapping = amici.petab.parameter_mapping.create_parameter_mapping(
    petab_problem=importer.petab_problem,
    simulation_conditions=simulation_conditions,
    scaled_parameters=True,
    amici_model=pypesto_problem.objective.amici_model,
    fill_fixed_parameters=True,
)

which is not great, because it requires me to have pretty deep knowledge of the code and requires a couple more lines of code than what I had hoped for.

Is there any specific reason why we are setting fill_fixed_parameters=True? If there is could we either expose this option to the user or set smarter defaults when this is really necessary?

Looks like this was introduced in https://github.com/ICB-DCM/pyPESTO/pull/912, but seems unrelated to the linked issue https://github.com/ICB-DCM/pyPESTO/issues/882. Unclear what "fix warning from fixed overrides" refers to.

dweindl commented 1 month ago

Sorry for the late reply.

Looks like this was introduced in #912, but seems unrelated to the linked issue #882.

The linked issue is unrelated to fill_fixed_parameters, correct. It was linked because that other change in this PR (https://github.com/ICB-DCM/pyPESTO/issues/882#issuecomment-1204463465).

Unclear what "fix warning from fixed overrides" refers to.

I don't recall that either. My guess would be: Before the change, pypesto was calling the amici/pypesto functions for filling parameter values according to the parameter mapping. The parameter mapping already set the values for the fixed parameters (fill_fixed_parameters=True). Thus, their symbols didn't show up in the parameter mapping anymore. Yet, pypesto still provided values for those parameters which triggered a warning that values were passed for unknown parameters. The PR was to prevent this warnings.

For anything based on the PEtab importer, changing fill_fixed_parameters should not be necessary.

this means that edata.plist will always be all parameters instead of subsetting to the ones that actually require sensitivities

This is very suprising. I am pretty sure this was working at some point and that there were tests in place. Needs further investigation.

dweindl commented 3 weeks ago

this means that edata.plist will always be all parameters instead of subsetting to the ones that actually require sensitivities

This is very suprising. I am pretty sure this was working at some point and that there were tests in place. Needs further investigation.

So my current understanding is:

dweindl commented 2 weeks ago

After a closer look: 1) plist is set correctly/efficiently if there are no pypesto-fixed parameters. I.e. condition-specific requirements are properly handled. 2) However, if there are pypesto-fixed parameters, they are not excluded from plist. For the latter I can't tell if this ever worked correctly or not.

I will try to get (2) fixed and will submit an effective test case shortly.

dweindl commented 2 weeks ago

2) However, if there are pypesto-fixed parameters, they are not excluded from plist. For the latter I can't tell if this ever worked correctly or not.

I am pretty confident this never worked.