Unspecified initial states in PEtab conditions table default to SBML initial value

[dilpath]

#1396

The warning added to "petab_import.py" is untested, just seems reasonable to me.

The warning added to "petab_objective.py" can result in many warnings, by default.

float() is used in petab_objective.py since any SBML initial assignment (for states in the PEtab conditions table) would have been overwritten in "petab_import.py" anyway, so I guess those overwritten SBML initial assignments should be ignored everywhere, hence only float-type values (initial amounts or concentrations) would be expected.

Not a nice solution I guess, since a private method from "sbml_import.py" is used.

[FFroehlich]

Is there an appropriate test case in the petab testsuite?

[FFroehlich]

Not a nice solution I guess, since a private method from "sbml_import.py" is used.

I don't think thats a big issue, feel free to make this a public method. I generally make all functions private by default as this slims the API and makes it easier to navigate.

[codecov]

Codecov Report

Merging #1397 (2a136c2) into develop (8c7aada) will increase coverage by 0.44%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop    #1397      +/-   ##
===========================================
+ Coverage    78.71%   79.16%   +0.44%     
===========================================
  Files           63       63              
  Lines        10004    10626     +622     
===========================================
+ Hits          7875     8412     +537     
- Misses        2129     2214      +85     

Flag	Coverage Δ
cpp	75.33% <ø> (+0.10%)	⬆️
petab	69.48% <66.66%> (-0.03%)	⬇️
python	67.72% <33.33%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/amici/petab_objective.py	89.61% <33.33%> (-0.94%)	⬇️
python/amici/petab_import.py	71.49% <100.00%> (+0.12%)	⬆️
python/amici/sbml_import.py	94.72% <100.00%> (-1.35%)	⬇️
python/amici/ode_export.py	90.03% <0.00%> (-2.74%)	⬇️
src/solver_cvodes.cpp	70.41% <0.00%> (+0.20%)	⬆️
src/sundials_matrix_wrapper.cpp	84.25% <0.00%> (+0.85%)	⬆️
src/exception.cpp	91.30% <0.00%> (+8.69%)	⬆️

[dilpath]

Is there an appropriate test case in the petab testsuite?

Not sure. @elbaraim could your PEtab problem from #1396 be added there?

[elbaraim]

Is there an appropriate test case in the petab testsuite?

Not sure. @elbaraim could your PEtab problem from #1396 be added there?

The PEtab example I provided in the issue? Sure. Although I think an even easier model could be used with the same characteristics as in that one. What do you think is more appropriate?

[elbaraim]

Hi @dilpath @dweindl ,

I think there is still an open issue in this PR:
If a preequilibration is defined and the initial state is left empty (or NaN) in the condition table, one would expect that the value that would be taken as initial would be the one obtained from the preequilibration and not from the SBML model.

For debugging: In the example I provided in #1396 one can see this unwanted behavior happening. The condition unperturbed is used as preequilibration condition but then the initial value (after preequilibration) is set to the initial value in the SBML (which is zero) instead of the corresponding value in sol['rdatas'].x_ss.

print('before reinitialization (preeq)')
print(sol['rdatas'][0].x_ss[15])
print(sol['rdatas'][1].x_ss[15])

447796069.1255652
447796069.1255652

print('after reinitialization')
print(sol['rdatas'][0].x0[15])
print(sol['rdatas'][1].x0[15])

2.0
0.0

For the conditions left empty (in this case unperturbed) I would expect to get the same value in x_ss and x0 (not zero in this case).

• Is this train of thoughts correct? Maybe I am just wrong 😅

[dweindl]

If a preequilibration is defined and the initial state is left empty (or NaN) in the condition table, one would expect that the value that would be taken as initial would be the one obtained from the preequilibration and not from the SBML model.

Right. The handling of species in the condition table needs a makeover. There is meanwhile a better test case in the petab test suite and I started fixing stuff (#1344). The problem is, that #1345 needs to be addressed first to fully support those PEtab cases there. This is still some work. But we should at least raise some exception in case things are not fully supported.

[sonarqubecloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

33.3% Coverage
0.0% Duplication

[dweindl]

Merging as is, even if it does not fully resolve @elbaraim 's issue. The rest will take a bit more time.