Add Problem.get_unused_id

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
 

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,

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.

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

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")