Skip to content

Conversation

@dweindl
Copy link
Member

@dweindl dweindl commented Feb 22, 2024

Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect.

For example, previously:
SBML parameter with value=2 -> will result in an amici parameter with that name
SBML parameter with initial assignment 2 -> will not result in an amici parameter with that name

As a first step: treat all SBML parameters that are initial assignment targets and whose initial assignment evaluates to a number as amici parameters.

Related to #2150.

@codecov
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.79%. Comparing base (4334557) to head (4939350).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2304      +/-   ##
===========================================
- Coverage    77.81%   77.79%   -0.03%     
===========================================
  Files          318      318              
  Lines        20534    20536       +2     
  Branches      1436     1436              
===========================================
- Hits         15979    15975       -4     
- Misses        4552     4558       +6     
  Partials         3        3              
Flag Coverage Δ
cpp 73.57% <100.00%> (-0.03%) ⬇️
cpp_python 34.01% <100.00%> (+<0.01%) ⬆️
petab 36.39% <100.00%> (+<0.01%) ⬆️
python 72.35% <100.00%> (-0.03%) ⬇️
sbmlsuite ∅ <ø> (∅)

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

Files Coverage Δ
python/sdist/amici/sbml_import.py 79.20% <100.00%> (+0.05%) ⬆️

... and 2 files with indirect coverage changes

dweindl added a commit to dweindl/AMICI that referenced this pull request Feb 22, 2024
Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect.

Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see AMICI-dev#2304) as amici expressions.
Those static expressions will be handled more efficiently after AMICI-dev#2303.

Related to AMICI-dev#2150.
@dweindl dweindl self-assigned this Feb 22, 2024
@dweindl dweindl marked this pull request as ready for review February 22, 2024 20:10
@dweindl dweindl requested a review from a team as a code owner February 22, 2024 20:10
Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model.
This is rather not what most users would expect.

As a first step: treat all SBML parameters that are initial assignment targets and whose initial assignment evaluates to a number as amici parameters.

Related to AMICI-dev#2150.
dweindl added a commit to dweindl/AMICI that referenced this pull request Feb 23, 2024
Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect.

Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see AMICI-dev#2304) as amici expressions.
Those static expressions will be handled more efficiently after AMICI-dev#2303.

Related to AMICI-dev#2150.
@dweindl dweindl added this pull request to the merge queue Feb 26, 2024
Merged via the queue into AMICI-dev:develop with commit 8271da1 Feb 26, 2024
@dweindl dweindl deleted the fix_2150_ia_params branch February 26, 2024 14:00
dweindl added a commit that referenced this pull request Feb 27, 2024
…2305)

Currently, parameters that are targets of initial assignments don't show up as parameters or expressions in the amici model. This is rather not what most users would expect.

Therefore, treat all SBML parameters that are initial assignment targets and whose initial assignment does not evaluate to a number (for those that do, see #2304) as amici expressions.
Those static expressions will be handled more efficiently after #2303.

Related to #2150.

See also #2304.
dweindl added a commit to dweindl/AMICI that referenced this pull request Mar 4, 2024
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 AMICI-dev#2304).
Now that those parameters remain in the model, they should be turned into
constant parameters, unless specified otherwise.
dweindl added a commit to dweindl/AMICI that referenced this pull request Mar 6, 2024
Evaluate initial assignments to parameters to determine whether the targets are amici parameters or expressions. Related to AMICI-dev#2304.
dweindl added a commit that referenced this pull request Mar 6, 2024
Evaluate initial assignments to parameters to determine whether the targets are amici parameters or expressions. Related to #2304.

For example, it the initial assignment is `log(10)`, this should still be a parameter instead of an expression, but `sympy.log(10).is_Number` will be `False`.
dweindl added a commit to dweindl/AMICI that referenced this pull request Mar 6, 2024
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 AMICI-dev#2304).
Now that those parameters remain in the model, they should be turned into
constant parameters, unless specified otherwise.
dweindl added a commit that referenced this pull request Mar 6, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SBML import: Undeclared identifier error when using parameters with initial assignments in observables

2 participants