Skip to content

Commit 38ba11e

Browse files
FFroehlichclaude
andcommitted
assignments_to_experiments: count conversion factors as uses, leave unconverted problems alone, pick an unused slope ID
- A parameter used as a model or species conversion factor counted as unused, so a condition change of it was dropped when the parameter also appeared in a converted assignment. Conversion factors now count as references. - A problem without time-dependent assignments was still changed: its experiments' periods were re-sorted. It is now returned as it was, after the lint. - A ramp's slope parameter was only named so as not to clash with SBML IDs, and clashed with, e.g., an observable parameter `<target>_slope`. It is now named with `Problem.get_unused_id`. `test_assignments_to_experiments_validate` relied on that slope clash to fail the lint; it now uses an existing experiment `default_experiment`, which the measurements without an experiment ID are assigned to as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 4824cb5 commit 38ba11e

2 files changed

Lines changed: 98 additions & 31 deletions

File tree

‎petab/v2/converters.py‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -820,14 +820,17 @@ def assignments_to_experiments(problem: Problem) -> Problem:
820820
expr = sbml_math_to_sympy(rule)
821821
if any(pw.has(TimeSymbol) for pw in expr.atoms(sp.Piecewise)):
822822
time_dependent.append((rule, expr))
823+
if not time_dependent:
824+
_check_lint(problem)
825+
return problem
823826

824827
experiments = list(problem.experiments)
825828
if any(m.experiment_id is None for m in problem.measurements):
826829
default_experiment = Experiment(id=DEFAULT_EXPERIMENT_ID)
827830
experiments.append(default_experiment)
828831
else:
829832
default_experiment = None
830-
droppable = _add_start_periods(experiments) if time_dependent else set()
833+
droppable = _add_start_periods(experiments)
831834

832835
# (experiment, period start time, target ID, target value)
833836
additions = []
@@ -876,15 +879,20 @@ def assignments_to_experiments(problem: Problem) -> Problem:
876879
if measurement.experiment_id is None:
877880
measurement.experiment_id = DEFAULT_EXPERIMENT_ID
878881

882+
_check_lint(problem)
883+
return problem
884+
885+
886+
def _check_lint(problem: Problem) -> None:
887+
"""Raise a :class:`ValueError` if the converted ``problem`` does not
888+
pass linting."""
879889
validation_issues = problem.validate()
880890
if validation_issues.has_errors():
881891
messages = "\n".join(f"{issue.message}" for issue in validation_issues)
882892
raise ValueError(
883893
f"The converted PEtab problem did not pass linting:\n{messages}"
884894
)
885895

886-
return problem
887-
888896

889897
class _PiecewiseAssignment:
890898
"""A time-dependent piecewise assignment to a model parameter, and the
@@ -964,9 +972,9 @@ def replace_in_model(self) -> None:
964972
target's, is set by every period that starts one.
965973
"""
966974
if self.ramps:
967-
self.slope_id = f"{self.target_id}_slope"
968-
while self.sbml_model.getElementBySId(self.slope_id) is not None:
969-
self.slope_id += "_"
975+
self.slope_id = self.problem.get_unused_id(
976+
f"{self.target_id}_slope"
977+
)
970978
add_sbml_parameter(
971979
self.sbml_model, self.slope_id, value=0.0, constant=False
972980
)
@@ -1143,10 +1151,11 @@ def _referenced_ids(
11431151
problem: Problem,
11441152
additions: list[tuple[Experiment, float, str, sp.Expr]],
11451153
) -> set[str]:
1146-
"""The IDs that the math of the model, the observables, or the condition
1147-
changes, including those in ``additions``, refer to."""
1154+
"""The IDs that the model, the observables, or the condition changes,
1155+
including those in ``additions``, refer to."""
1156+
sbml_model = problem.model.sbml_model
11481157
ids = set()
1149-
for element in problem.model.sbml_model.getListOfAllElements():
1158+
for element in sbml_model.getListOfAllElements():
11501159
if not (hasattr(element, "isSetMath") and element.isSetMath()):
11511160
continue
11521161
nodes = [element.getMath()]
@@ -1155,6 +1164,12 @@ def _referenced_ids(
11551164
if node.getType() == libsbml.AST_NAME:
11561165
ids.add(node.getName())
11571166
nodes += [node.getChild(i) for i in range(node.getNumChildren())]
1167+
# conversion factors refer to parameters by attribute, not in math
1168+
ids |= {
1169+
element.getConversionFactor()
1170+
for element in [sbml_model, *sbml_model.getListOfSpecies()]
1171+
if element.isSetConversionFactor()
1172+
}
11581173
expressions = [
11591174
*(o.formula for o in problem.observables),
11601175
*(o.noise_formula for o in problem.observables),

‎tests/v2/test_conversion.py‎

Lines changed: 74 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1477,27 +1477,22 @@ def test_assignments_to_experiments_preequilibration_boundary(
14771477
def test_assignments_to_experiments_validate():
14781478
"""Test that the converted problem is linted.
14791479
1480-
Here, the input problem is valid, but the conversion adds the model
1481-
parameter `k1_slope` for the ramp, which is also an estimated observable
1482-
parameter in the parameter table.
1480+
Here, the input problem is valid, but the measurement without an
1481+
experiment ID is assigned to a new experiment `default_experiment`, an ID
1482+
that an existing experiment already has.
14831483
"""
14841484
problem = Problem()
1485-
ant_model = """
1486-
model conversion
1487-
species A, B;
1488-
A = 10;
1489-
B = 0;
1490-
k1 := piecewise(time, time < 10, 1);
1491-
k2 = 0.5;
1492-
R1: A -> B; k1 * A;
1493-
R2: B -> A; k2 * B;
1494-
end
1495-
"""
1496-
problem.model = SbmlModel.from_antimony(ant_model)
1497-
problem.add_observable("obs_a", formula="k1_slope * A", noise_formula=1.0)
1498-
problem.add_measurement("obs_a", time=20.0, measurement=5.0)
1499-
problem.add_parameter("k1_slope", estimate=True, lb=1e-3, ub=1e3)
1500-
problem.add_parameter("k2", estimate=True, lb=1e-3, ub=1e3)
1485+
problem.model = _switch_model("piecewise(0, time < 10, 1)")
1486+
problem.add_condition("c1", other=2.0)
1487+
problem.add_experiment(DEFAULT_EXPERIMENT_ID, 0, "c1")
1488+
problem.add_observable("obs_a", formula="A", noise_formula=1.0)
1489+
problem.add_measurement(
1490+
"obs_a",
1491+
experiment_id=DEFAULT_EXPERIMENT_ID,
1492+
time=20.0,
1493+
measurement=5.0,
1494+
)
1495+
problem.add_measurement("obs_a", time=30.0, measurement=5.0)
15011496
assert not problem.validate().has_errors()
15021497

15031498
with pytest.raises(ValueError, match="did not pass linting"):
@@ -1661,7 +1656,8 @@ def test_assignments_to_experiments_species_target():
16611656

16621657
def test_assignments_to_experiments_nothing_to_convert():
16631658
"""Test that a problem without time-dependent assignments is unchanged,
1664-
including an experiment that only preequilibrates."""
1659+
including an experiment that only preequilibrates and periods that are
1660+
not in time order."""
16651661
problem = Problem()
16661662
problem.model = SbmlModel.from_antimony("""
16671663
model conversion
@@ -1672,13 +1668,69 @@ def test_assignments_to_experiments_nothing_to_convert():
16721668
end
16731669
""")
16741670
problem.add_condition("c1", k2=1.0)
1671+
problem.add_condition("c2", k2=2.0)
16751672
problem.add_experiment("e1", v2.C.TIME_PREEQUILIBRATION, "c1")
1673+
# periods out of time order
1674+
problem.add_experiment("e2", 10, "c2", 0, "c1")
1675+
problem.add_observable("obs_a", formula="A", noise_formula=1.0)
1676+
for experiment_id in ("e1", "e2"):
1677+
problem.add_measurement(
1678+
"obs_a", experiment_id=experiment_id, time=20.0, measurement=1.0
1679+
)
1680+
1681+
converted = assignments_to_experiments(problem)
1682+
1683+
def periods(problem):
1684+
return [
1685+
[(p.time, p.condition_ids) for p in e.periods]
1686+
for e in problem.experiments
1687+
]
1688+
1689+
assert periods(converted) == periods(problem)
1690+
assert converted.condition_df.equals(problem.condition_df)
1691+
1692+
1693+
@pytest.mark.parametrize("on", ["model", "species"])
1694+
def test_assignments_to_experiments_conversion_factor(on):
1695+
"""Test that the changes of a symbol of a converted assignment stay if
1696+
the model refers to that symbol as a conversion factor."""
1697+
problem = Problem()
1698+
problem.model = _switch_model(
1699+
"cf * piecewise(0, time < 10, 1)", extra="cf = 1;"
1700+
)
1701+
sbml_model = problem.model.sbml_model
1702+
element = sbml_model if on == "model" else sbml_model.getSpecies("A")
1703+
assert (
1704+
element.setConversionFactor("cf") == libsbml.LIBSBML_OPERATION_SUCCESS
1705+
)
1706+
problem.add_condition("c1", cf=2.0)
1707+
problem.add_experiment("e1", 0, "c1")
16761708
problem.add_observable("obs_a", formula="A", noise_formula=1.0)
16771709
problem.add_measurement(
16781710
"obs_a", experiment_id="e1", time=20.0, measurement=1.0
16791711
)
16801712

16811713
converted = assignments_to_experiments(problem)
16821714

1683-
assert converted.experiment_df.equals(problem.experiment_df)
1684-
assert converted.condition_df.equals(problem.condition_df)
1715+
assert _changes_by_time(converted, "e1") == {
1716+
0.0: {"cf": 2.0},
1717+
10.0: {"k1": 2.0},
1718+
}
1719+
1720+
1721+
def test_assignments_to_experiments_slope_id_taken():
1722+
"""Test that a ramp's slope parameter gets an ID that the problem does not
1723+
use yet, here because an observable parameter is called `k1_slope`."""
1724+
problem = Problem()
1725+
problem.model = _switch_model("piecewise(time, time < 10, 1)")
1726+
problem.add_observable("obs_a", formula="k1_slope * A", noise_formula=1.0)
1727+
problem.add_measurement("obs_a", time=20.0, measurement=5.0)
1728+
problem.add_parameter("k1_slope", estimate=True, lb=1e-3, ub=1e3)
1729+
1730+
converted = assignments_to_experiments(problem)
1731+
1732+
assert _rate_rule(converted, "k1") == "k1_slope_1"
1733+
assert _changes_by_time(converted, DEFAULT_EXPERIMENT_ID) == {
1734+
0.0: {"k1": 0.0, "k1_slope_1": 1.0},
1735+
10.0: {"k1": 1.0, "k1_slope_1": 0.0},
1736+
}

0 commit comments

Comments
 (0)