PEtab-dev / libpetab-python

Python package for working with PEtab files
https://libpetab-python.readthedocs.io
MIT License
14 stars 5 forks source link

Fix `SbmlModel.get_free_parameter_ids_with_values` to handle InitialA… #248

Closed dweindl closed 4 months ago

dweindl commented 4 months ago

…ssignments

So far, SbmlModel.get_free_parameter_ids_with_values ignored InitialAssignments, resulting in incorrect parameter values in the parameter mapping.

Now the correct values are used in case of initial assignments that are parameter-independent, and parameters with other initial assignments are ignored.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 76.21%. Comparing base (7e24e14) to head (c47666d).

Files Patch % Lines
petab/models/sbml_model.py 36.36% 6 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #248 +/- ## =========================================== - Coverage 76.34% 76.21% -0.14% =========================================== Files 34 34 Lines 3205 3216 +11 Branches 779 780 +1 =========================================== + Hits 2447 2451 +4 - Misses 555 561 +6 - Partials 203 204 +1 ```

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

dweindl commented 4 months ago

Why does a free parameter take a value from an SBML assignment rule? Shouldn't values for free parameters come from an optimizer? I would expect that initial assignments/values for free parameters are somehow removed, is that what this method is for?

Assignment rule targets are excluded here.

The method name is maybe not ideal. In the end, this method is used to collect parameters and default values that should be present in the parameter mapping. I guess it's up for discussion what should be included/excluded there. However, the original situation was clearly bad: For example, a parameter with value=0 and initialAssignment of 2 would have been included there with a value of 0 instead of 2. For amici that didn't matter until recently (https://github.com/AMICI-dev/AMICI/pull/2304) due to its handling of initialAssignments, but for any other tool this would have been a problem. Whether or not initialAssignments that depend on other parameters should be included here is a different question. Not sure about that. Including them, would however require more widespread changes, since then the parameter mapping would contain symbolic expressions. (Which would happen anyways if we proceed with the timecourse proposal).

dilpath commented 4 months ago

Why does a free parameter take a value from an SBML assignment rule? Shouldn't values for free parameters come from an optimizer? I would expect that initial assignments/values for free parameters are somehow removed, is that what this method is for?

Assignment rule targets are excluded here.

Meant to say initial assignment rule there, but I see now that this is not about free parameters only

The method name is maybe not ideal. In the end, this method is used to collect parameters and default values that should be present in the parameter mapping. I guess it's up for discussion what should be included/excluded there. However, the original situation was clearly bad: For example, a parameter with value=0 and initialAssignment of 2 would have been included there with a value of 0 instead of 2. For amici that didn't matter until recently (AMICI-dev/AMICI#2304) due to its handling of initialAssignments, but for any other tool this would have been a problem. Whether or not initialAssignments that depend on other parameters should be included here is a different question. Not sure about that. Including them, would however require more widespread changes, since then the parameter mapping would contain symbolic expressions. (Which would happen anyways if we proceed with the timecourse proposal).

OK, sounds good to me then. Doing SymPy calls frequently sounds inefficient, but maybe JIT/eval/exec can be used to turn them into a standard Python function on the fly.

dweindl commented 4 months ago

Doing SymPy calls frequently sounds inefficient, but maybe JIT/eval/exec can be used to turn them into a standard Python function on the fly.

This is happening once during the parameter mapping. I don't think it really matters, unless the model is really huge, and even then, this is probably not the major problem. I can try float(.) first to handle the super basic cases first without sympy, but I think it won't matter mach.

dilpath commented 4 months ago

Doing SymPy calls frequently sounds inefficient, but maybe JIT/eval/exec can be used to turn them into a standard Python function on the fly.

This is happening once during the parameter mapping. I don't think it really matters, unless the model is really huge, and even then, this is probably not the major problem. I can try float(.) first to handle the super basic cases first without sympy, but I think it won't matter mach.

OK, sounds reasonable to me; only initial assignments without parameter dependencies are handled.