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

dweindl · 2024-03-05T19:59:26Z

…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.

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

codecov-commenter · 2024-03-05T20:03:03Z

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 ⚠️

Additional details and impacted files

@@             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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dilpath

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?

dilpath · 2024-03-05T21:05:13Z

petab/models/sbml_model.py

+        def get_initial(p):
+            # return the initial assignment value if there is one, and it is a
+            # number, otherwise the parameter value
+            if ia := self.sbml_model.getInitialAssignmentBySymbol(p.getId()):


Suggested change

if ia := self.sbml_model.getInitialAssignmentBySymbol(p.getId()):

if ia := self.sbml_model.getInitialAssignmentBySymbol(p.getId()) is not None:

That's implied.

👍 was just a PEP 8 recommendation

dilpath · 2024-03-05T21:14:05Z

petab/models/sbml_model.py

@@ -103,8 +104,31 @@ def get_free_parameter_ids_with_values(
            ar.getVariable() for ar in self.sbml_model.getListOfRules()
        }

+        parser_settings = libsbml.L3ParserSettings(
+            self.sbml_model,
+            libsbml.L3P_PARSE_LOG_AS_LOG10,


This is pretty unintuitive, to me it looks like this will convert log to log10, and I'm not sure if the libsbml docs make sense. e.g. this for L3P_PARSE_LOG_AS_ERROR

Collapse unary minuses where possible when parsing text-string formulas.

https://synonym.caltech.edu/software/libsbml/5.18.0/docs/formatted/python-api/namespacelibsbml.html#a9ef0398726fb45bd1cb24c4a5ed671f0

The docstring for L3P_PARSE_LOG_AS_LOG10 looks like it actually parses log as log, not parsing log as log10, but I'm not sure I trust the docs given the docstring for L3P_PARSE_LOG_AS_ERROR. Should we instead use L3P_PARSE_LOG_AS_LN?

I think most users would expect log to be ln, and that log10 is only when explicitly specified with log(10, x) or log10(x).

Those are the libsbml default settings. The only relevant thing here is ignoring units.

OK, if it actually keeps log as log, not log10, then no problem

Note that this is about parsing MathML, not any observableFormula or the like. In MathML log without explicit base means log10. In plain text formulae, log will be interpreted as ln.

Oh! Good to know that log means different things in PEtab and the model 😬 Then I think we could be more careful about a math spec in the next version of PEtab, I added a comment to the draft timecourse spec.

dweindl · 2024-03-06T07:33:48Z

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 (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).

dilpath · 2024-03-06T10:09:20Z

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 · 2024-03-06T11:47:17Z

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 · 2024-03-06T12:08:20Z

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.

…2345) During PEtab import, parameters that are targets of initial assignments have so far not been turned into constant parameters, because they didn't exist in the amici model (see #2304). Now that those parameters remain in the model, they should be turned into constant parameters, unless specified otherwise. Requires PEtab-dev/libpetab-python#248, otherwise PEtab parameter mapping will provide potentially incorrect values for those parameters.

Fix SbmlModel.get_free_parameter_ids_with_values to handle InitialA…

15d210a

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

dweindl mentioned this pull request Mar 5, 2024

Add test case with parameters with initial assignments PEtab-dev/petab_test_suite#61

Open

dweindl requested a review from dilpath March 5, 2024 20:45

dweindl marked this pull request as ready for review March 5, 2024 20:46

dweindl requested a review from a team as a code owner March 5, 2024 20:46

dilpath approved these changes Mar 5, 2024

View reviewed changes

dweindl mentioned this pull request Mar 6, 2024

Fix petab import: initial assignment targets as constant parameters AMICI-dev/AMICI#2345

Merged

float; omit non-numeric initial assignments

c47666d

dweindl merged commit 8a10ace into develop Mar 6, 2024
7 checks passed

dweindl deleted the fix_free_pars_ia branch March 6, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dweindl commented Mar 5, 2024 •

edited

Loading

codecov-commenter commented Mar 5, 2024 •

edited

Loading

dilpath left a comment

dilpath Mar 5, 2024

dweindl Mar 6, 2024

dilpath Mar 6, 2024

dilpath Mar 5, 2024

dweindl Mar 6, 2024

dilpath Mar 6, 2024

dweindl Mar 6, 2024 •

edited

Loading

dilpath Mar 6, 2024

dweindl commented Mar 6, 2024 •

edited

Loading

dilpath commented Mar 6, 2024

dweindl commented Mar 6, 2024

dilpath commented Mar 6, 2024

	if ia := self.sbml_model.getInitialAssignmentBySymbol(p.getId()):
	if ia := self.sbml_model.getInitialAssignmentBySymbol(p.getId()) is not None:

Fix SbmlModel.get_free_parameter_ids_with_values to handle InitialA… #248

Fix SbmlModel.get_free_parameter_ids_with_values to handle InitialA… #248

Conversation

dweindl commented Mar 5, 2024 • edited Loading

codecov-commenter commented Mar 5, 2024 • edited Loading

Codecov Report

dilpath left a comment

Choose a reason for hiding this comment

dilpath Mar 5, 2024

Choose a reason for hiding this comment

dweindl Mar 6, 2024

Choose a reason for hiding this comment

dilpath Mar 6, 2024

Choose a reason for hiding this comment

dilpath Mar 5, 2024

Choose a reason for hiding this comment

dweindl Mar 6, 2024

Choose a reason for hiding this comment

dilpath Mar 6, 2024

Choose a reason for hiding this comment

dweindl Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

dilpath Mar 6, 2024

Choose a reason for hiding this comment

dweindl commented Mar 6, 2024 • edited Loading

dilpath commented Mar 6, 2024

dweindl commented Mar 6, 2024

dilpath commented Mar 6, 2024

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

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

dweindl commented Mar 5, 2024 •

edited

Loading

codecov-commenter commented Mar 5, 2024 •

edited

Loading

dweindl Mar 6, 2024 •

edited

Loading

dweindl commented Mar 6, 2024 •

edited

Loading