diff --git a/CHANGELOG.md b/CHANGELOG.md index 87ad83c9..b1398e4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,9 @@ each `Change`, which caused renamed conditions to be written with their old ID (by @dweindl in https://github.com/PEtab-dev/libpetab-python/pull/551) +* `assignments_to_experiments` no longer creates condition IDs that clash + with other IDs in the problem + (by @dweindl in https://github.com/PEtab-dev/libpetab-python/pull/561) **Features** * `petab1to2` and `petab_files_1to2` can skip validation via `validate=False` @@ -63,6 +66,9 @@ `Problem.to_files(columns=...)`, or globally via `petab.v2.set_options` and `petab.v2.option_context` (by @dweindl in https://github.com/PEtab-dev/libpetab-python/pull/543) +* `petab.v2.Problem.get_unused_id` returns an identifier that is not yet + used in the problem or its model + (by @dweindl in https://github.com/PEtab-dev/libpetab-python/pull/561) ## 0.9 series diff --git a/petab/v2/converters.py b/petab/v2/converters.py index 8dc710f2..37c5d001 100644 --- a/petab/v2/converters.py +++ b/petab/v2/converters.py @@ -4,7 +4,6 @@ import warnings from copy import deepcopy -from itertools import count import libsbml import sympy as sp @@ -892,11 +891,9 @@ def assignments_to_experiments(problem: Problem) -> Problem: # add the new conditions to new or existing periods new_conditions = {} - cond_counter = count() for experiment, t, target_id, value in additions: if (cond_id := new_conditions.get((target_id, value))) is None: - while (cond_id := f"cond_{next(cond_counter)}") in condition_ids: - pass + cond_id = problem.get_unused_id(f"cond_{len(new_conditions)}") new_conditions[target_id, value] = cond_id problem += Condition( id=cond_id, diff --git a/petab/v2/core.py b/petab/v2/core.py index 3a5cf86d..f69a16ed 100644 --- a/petab/v2/core.py +++ b/petab/v2/core.py @@ -8,7 +8,7 @@ import tempfile import traceback from abc import abstractmethod -from collections.abc import Sequence +from collections.abc import Collection, Sequence from enum import StrEnum from itertools import chain from math import nan @@ -1800,6 +1800,72 @@ def id(self, value: str): self.config = ProblemConfig(format_version="2.0.0") self.config.id = value + def get_unused_id( + self, base: str, *, exclude: Collection[str] = () + ) -> str: + """Get an identifier that is not used in the problem yet. + + Returns `base` if it is unused, otherwise the first unused + `base_1`, `base_2`, .... + An identifier is used if it is a PEtab entity ID, a condition target, + a model entity ID, or a symbol in any expression or override. + + :param base: The preferred identifier. + :param exclude: Additional identifiers to treat as used, e.g., IDs + already chosen for entities that are not added to the problem yet. + :return: The unused identifier. + """ + if not is_valid_identifier(base): + raise ValueError(f"`{base}' is not a valid identifier.") + + used_ids = self._get_used_ids() + + def is_used(id_: str) -> bool: + return ( + id_ in used_ids + or id_ in exclude + or any(m.has_entity_with_id(id_) for m in self.models) + ) + + id_ = base + i = 0 + while is_used(id_): + i += 1 + id_ = f"{base}_{i}" + return id_ + + def _get_used_ids(self) -> set[str]: + """Get all IDs and symbols used in the PEtab tables.""" + ids = {p.id for p in self.parameters} + ids |= {o.id for o in self.observables} + ids |= {c.id for c in self.conditions} + ids |= {e.id for e in self.experiments} + ids |= {m.petab_id for m in self.mappings} + ids |= {ch.target_id for c in self.conditions for ch in c.changes} + expressions = chain( + chain.from_iterable( + ( + *o.observable_placeholders, + *o.noise_placeholders, + o.formula, + o.noise_formula, + ) + for o in self.observables + ), + (ch.target_value for c in self.conditions for ch in c.changes), + chain.from_iterable( + chain(m.observable_parameters, m.noise_parameters) + for m in self.measurements + ), + ) + ids |= { + str(s) + for expr in expressions + if expr is not None + for s in expr.free_symbols + } + return ids + def get_optimization_parameters(self) -> list[str]: """ Get the list of optimization parameter IDs from parameter table. diff --git a/tests/v2/test_conversion.py b/tests/v2/test_conversion.py index 8cdc5237..7fd8fd96 100644 --- a/tests/v2/test_conversion.py +++ b/tests/v2/test_conversion.py @@ -711,6 +711,24 @@ def test_assignments_to_experiments(condition, before, after): ] +def test_assignments_to_experiments_unused_condition_id(): + """Test that new condition IDs don't clash with other IDs.""" + problem = Problem() + problem.model = SbmlModel.from_antimony(""" + species A = 10; + k1 = piecewise(1, time < 10, 2); + R1: A -> ; k1 * A; + """) + problem.add_observable("obs_a", formula="A", noise_formula="cond_0") + problem.add_measurement("obs_a", time=20.0, measurement=5.0) + problem.add_parameter("cond_0", estimate=True, lb=1e-3, ub=1e3) + + converted = assignments_to_experiments(problem) + + assert [c.id for c in converted.conditions] == ["cond_0_1"] + assert converted.experiments[0].periods[0].condition_ids == ["cond_0_1"] + + def test_assignments_to_experiments_nested(): """Test conversion of piecewise assignments that are nested inside a larger expression and switch at a time given by some expression.""" diff --git a/tests/v2/test_core.py b/tests/v2/test_core.py index 5a163f19..5f95ed15 100644 --- a/tests/v2/test_core.py +++ b/tests/v2/test_core.py @@ -1550,3 +1550,66 @@ def test_problem_to_files_columns(tmp_path): C.OBSERVABLE_FORMULA, C.NOISE_FORMULA, ] + + +def _add_override_x(problem: Problem): + problem.add_observable( + "o", + "observableParameter1_o", + noise_formula=1, + observable_placeholders=["observableParameter1_o"], + ) + problem.add_measurement( + "o", time=1, measurement=1, observable_parameters=["x"] + ) + + +def _add_model_x(problem: Problem): + problem.model = SbmlModel.from_antimony("species x = 1") + + +@pytest.mark.parametrize( + "add_x", + [ + pytest.param( + lambda p: p.add_parameter("x", False, nominal_value=1), + id="parameter", + ), + pytest.param( + lambda p: p.add_observable("x", "1", noise_formula=1), + id="observable", + ), + pytest.param(lambda p: p.add_condition("x", k=1), id="condition"), + pytest.param(lambda p: p.add_experiment("x", 0, ""), id="experiment"), + pytest.param(lambda p: p.add_mapping("x", "k.1"), id="mapping"), + pytest.param(lambda p: p.add_condition("c", x=1), id="target_id"), + pytest.param( + lambda p: p.add_condition("c", k="2 * x"), id="target_value" + ), + pytest.param( + lambda p: p.add_observable("o", "2 * x", noise_formula=1), + id="observable_formula", + ), + pytest.param( + lambda p: p.add_observable("o", "1", noise_formula="x"), + id="noise_formula", + ), + pytest.param(_add_override_x, id="override"), + pytest.param(_add_model_x, id="model_entity"), + ], +) +def test_get_unused_id(add_x): + """Test that IDs used anywhere in the problem are avoided.""" + problem = Problem() + assert problem.get_unused_id("x") == "x" + + add_x(problem) + assert problem.get_unused_id("x") == "x_1" + assert problem.get_unused_id("x", exclude={"x_1"}) == "x_2" + assert problem.get_unused_id("y") == "y" + + +def test_get_unused_id_invalid(): + """Test that invalid base IDs are rejected.""" + with pytest.raises(ValueError, match="not a valid identifier"): + Problem().get_unused_id("1x")