From 17e118b50ce5df0dd4fcfd8a1050df3a1c01ed2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Fr=C3=B6hlich?= Date: Thu, 8 Oct 2026 13:29:48 +0100 Subject: [PATCH 1/2] petab1to2: order placeholders by index, not as text v1 observable/noise parameter overrides are positional, but the v2 `observablePlaceholders` / `noisePlaceholders` were sorted as strings, so from 10 placeholders on (`observableParameter10_x` < `observableParameter2_x`) the overrides were silently assigned to the wrong placeholders. Sort by the numeric index instead, and raise a ValueError if the placeholders are not numbered consecutively from 1, since positional overrides cannot be mapped then (v1 lint already rejects this). Closes #531 Co-Authored-By: Claude Opus 5.5 --- petab/v2/petab1to2.py | 28 ++++++---- tests/v2/test_conversion.py | 107 ++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 11 deletions(-) diff --git a/petab/v2/petab1to2.py b/petab/v2/petab1to2.py index 14ed8939..2cb31fed 100644 --- a/petab/v2/petab1to2.py +++ b/petab/v2/petab1to2.py @@ -438,20 +438,26 @@ def extract_placeholders(row: pd.Series, type_: str) -> str: t = f"{re.escape(type_)}Parameter" o = re.escape(row[v1.C.OBSERVABLE_ID]) - pattern = re.compile(rf"(?:^|\W)({t}\d+_{o})(?=\W|$)") + pattern = re.compile(rf"(?:^|\W)({t}(\d+)_{o})(?=\W|$)") expr = sympify_petab(formula) - # for 10+ placeholders, the current lexicographical sorting will result - # in incorrect ordering of the placeholder IDs, so that they don't - # align with the overrides in the measurement table, but who does - # that anyway? - return v2.C.PARAMETER_SEPARATOR.join( - sorted( - str(sym) - for sym in expr.free_symbols - if sym.is_Symbol and pattern.match(str(sym)) - ) + # v1 overrides are positional: the i-th override replaces + # `{type_}Parameter{i}_{observableId}`. Therefore, sort by index + # (not as text, which would put 10 before 2) and require 1..n. + placeholders = sorted( + (int(match.group(2)), str(sym)) + for sym in expr.free_symbols + if sym.is_Symbol and (match := pattern.match(str(sym))) ) + if [i for i, _ in placeholders] != list( + range(1, len(placeholders) + 1) + ): + raise ValueError( + f"The {type_} placeholders of observable " + f"`{row[v1.C.OBSERVABLE_ID]}' are not numbered consecutively " + f"starting from 1: {[p for _, p in placeholders]}." + ) + return v2.C.PARAMETER_SEPARATOR.join(p for _, p in placeholders) df[v2.C.OBSERVABLE_PLACEHOLDERS] = df.apply( extract_placeholders, args=("observable",), axis=1 diff --git a/tests/v2/test_conversion.py b/tests/v2/test_conversion.py index 3380c363..4b093978 100644 --- a/tests/v2/test_conversion.py +++ b/tests/v2/test_conversion.py @@ -29,6 +29,113 @@ def test_v1v2_observable_df_noise_distribution(): ] +def test_petab1to2_placeholder_order(tmp_path): + """Test that placeholders are ordered by index, not as text, so that the + positional overrides still match for 10+ placeholders.""" + obs_id = "obs_B" + n_obs, n_noise = 12, 11 + observable_df = pd.DataFrame( + data={ + v1.C.OBSERVABLE_ID: [obs_id], + v1.C.OBSERVABLE_FORMULA: [ + " + ".join( + f"observableParameter{i}_{obs_id} * B^{i}" + for i in range(1, n_obs + 1) + ) + ], + v1.C.NOISE_FORMULA: [ + " + ".join( + f"noiseParameter{i}_{obs_id}" + for i in range(1, n_noise + 1) + ) + ], + } + ).set_index(v1.C.OBSERVABLE_ID) + # the i-th observable override is i, the i-th noise override is 100 + i + measurement_df = pd.DataFrame( + data={ + v1.C.OBSERVABLE_ID: [obs_id, obs_id], + v1.C.SIMULATION_CONDITION_ID: ["c0", "c0"], + v1.C.TIME: [1.0, 2.0], + v1.C.MEASUREMENT: [0.3, 0.5], + v1.C.OBSERVABLE_PARAMETERS: ";".join( + str(i) for i in range(1, n_obs + 1) + ), + v1.C.NOISE_PARAMETERS: ";".join( + str(100 + i) for i in range(1, n_noise + 1) + ), + } + ) + condition_df = pd.DataFrame( + data={v1.C.CONDITION_ID: ["c0"], "A": [1.0]} + ).set_index(v1.C.CONDITION_ID) + parameter_df = pd.DataFrame( + data={ + v1.C.PARAMETER_ID: ["k1"], + v1.C.PARAMETER_SCALE: [v1.C.LIN], + v1.C.LOWER_BOUND: [1e-3], + v1.C.UPPER_BOUND: [1e3], + v1.C.NOMINAL_VALUE: [0.5], + v1.C.ESTIMATE: [1], + } + ).set_index(v1.C.PARAMETER_ID) + v1_problem = v1.Problem( + model=v1.models.sbml_model.SbmlModel.from_antimony( + "A -> B; k1 * A; A = 1; B = 0; k1 = 0.5" + ), + condition_df=condition_df, + measurement_df=measurement_df, + parameter_df=parameter_df, + observable_df=observable_df, + ) + + problem = petab1to2(v1_problem.to_files_generic(tmp_path)) + + observable = problem[obs_id] + assert len(problem.measurements) == 2 + for measurement in problem.measurements: + assert { + str(placeholder): float(override) + for placeholder, override in zip( + observable.observable_placeholders, + measurement.observable_parameters, + strict=True, + ) + } == { + f"observableParameter{i}_{obs_id}": i for i in range(1, n_obs + 1) + } + assert { + str(placeholder): float(override) + for placeholder, override in zip( + observable.noise_placeholders, + measurement.noise_parameters, + strict=True, + ) + } == { + f"noiseParameter{i}_{obs_id}": 100 + i + for i in range(1, n_noise + 1) + } + + +@pytest.mark.parametrize("type_", ["observable", "noise"]) +def test_v1v2_observable_df_placeholder_gap(type_): + """Test that non-consecutively numbered placeholders are rejected, + because v1 overrides are positional.""" + formula = f"{type_}Parameter1_obs1 + {type_}Parameter3_obs1" + observable_df = pd.DataFrame( + data={ + v1.C.OBSERVABLE_ID: ["obs1"], + v1.C.OBSERVABLE_FORMULA: [ + formula if type_ == "observable" else "a" + ], + v1.C.NOISE_FORMULA: [formula if type_ == "noise" else "1"], + } + ).set_index(v1.C.OBSERVABLE_ID) + + with pytest.raises(ValueError, match="not numbered consecutively"): + v1v2_observable_df(observable_df) + + def test_petab1to2_remote(): """Test that we can upgrade a remote PEtab 1.0.0 problem.""" yaml_url = ( From bfaf9e43eb57034ff2eda1cfb56ea65f44871dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20Fr=C3=B6hlich?= Date: Thu, 8 Oct 2026 18:50:10 +0100 Subject: [PATCH 2/2] petab1to2: reuse v1 get_formula_placeholders for placeholder extraction Use the helper that the v1 linter and the v1 override-count check use, instead of re-implementing the index ordering and the consecutive-numbering check. This also makes the placeholders consistent with v1 when sympy would cancel a placeholder from the formula. Co-Authored-By: Claude Opus 5.5 --- petab/v2/petab1to2.py | 34 +++++++++++----------------------- tests/v2/test_conversion.py | 2 +- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/petab/v2/petab1to2.py b/petab/v2/petab1to2.py index 2cb31fed..6097fbbd 100644 --- a/petab/v2/petab1to2.py +++ b/petab/v2/petab1to2.py @@ -2,7 +2,6 @@ from __future__ import annotations -import re import shutil import warnings from contextlib import suppress @@ -15,7 +14,6 @@ from pandas.io.common import get_handle, is_url from .. import v1, v2 -from ..v1.math import sympify_petab from ..v1.yaml import get_path_prefix, load_yaml, validate from ..versions import get_major_version from .models import MODEL_TYPE_SBML @@ -435,29 +433,19 @@ def extract_placeholders(row: pd.Series, type_: str) -> str: if pd.isna(formula): return "" - t = f"{re.escape(type_)}Parameter" - o = re.escape(row[v1.C.OBSERVABLE_ID]) - - pattern = re.compile(rf"(?:^|\W)({t}(\d+)_{o})(?=\W|$)") - - expr = sympify_petab(formula) # v1 overrides are positional: the i-th override replaces - # `{type_}Parameter{i}_{observableId}`. Therefore, sort by index - # (not as text, which would put 10 before 2) and require 1..n. - placeholders = sorted( - (int(match.group(2)), str(sym)) - for sym in expr.free_symbols - if sym.is_Symbol and (match := pattern.match(str(sym))) - ) - if [i for i, _ in placeholders] != list( - range(1, len(placeholders) + 1) - ): - raise ValueError( - f"The {type_} placeholders of observable " - f"`{row[v1.C.OBSERVABLE_ID]}' are not numbered consecutively " - f"starting from 1: {[p for _, p in placeholders]}." + # `{type_}Parameter{i}_{observableId}`. This returns the placeholders + # ordered by index and fails if they are not numbered 1..n. + try: + placeholders = v1.get_formula_placeholders( + formula, row[v1.C.OBSERVABLE_ID], type_ ) - return v2.C.PARAMETER_SEPARATOR.join(p for _, p in placeholders) + except AssertionError as e: + raise ValueError( + f"Cannot convert the {type_} placeholders of observable " + f"`{row[v1.C.OBSERVABLE_ID]}': {e}" + ) from e + return v2.C.PARAMETER_SEPARATOR.join(placeholders) df[v2.C.OBSERVABLE_PLACEHOLDERS] = df.apply( extract_placeholders, args=("observable",), axis=1 diff --git a/tests/v2/test_conversion.py b/tests/v2/test_conversion.py index 4b093978..f51d90bf 100644 --- a/tests/v2/test_conversion.py +++ b/tests/v2/test_conversion.py @@ -132,7 +132,7 @@ def test_v1v2_observable_df_placeholder_gap(type_): } ).set_index(v1.C.OBSERVABLE_ID) - with pytest.raises(ValueError, match="not numbered consecutively"): + with pytest.raises(ValueError, match="Non-consecutive numbering"): v1v2_observable_df(observable_df)