Address review comments on log reparametrization

* `petab1to2`: also warn about converted `parameterScaleUniform` priors
  with `reparametrize="none"`, which change the maximum a posteriori
  estimate as well
* `petab1to2`: point to `reparametrize="none"` if reparametrization
  fails, and drop the `parameter_ids` hint from the
  `reparametrize_log` error, which doesn't apply there
* `petab1to2`: warn once about parameters skipped because of
  placeholders, not once per base
* `reparametrize_log`: ignore non-positive bounds of non-estimated
  parameters, and raise `NotImplementedError` for SBML models before
  L2V2, which have no initial assignments
* tests: likelihood after reparametrizing overrides with both bases,
  and the benchmark collection with `reparametrize="all"`

Generated with AI assistance.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: dweindl

petab/v2/petab1to2.py

@@ -701,7 +701,11 @@ def update_prior(row) -> tuple[str, str | None]:
         has_scale_prior = parameter_df[v1.C.PARAMETER_SCALE].isin(
             [v1.C.LOG, v1.C.LOG10]
         ) & parameter_df[v1.C.OBJECTIVE_PRIOR_TYPE].isin(
-            [v1.C.PARAMETER_SCALE_NORMAL, v1.C.PARAMETER_SCALE_LAPLACE]
+            [
+                v1.C.PARAMETER_SCALE_NORMAL,
+                v1.C.PARAMETER_SCALE_LAPLACE,
+                v1.C.PARAMETER_SCALE_UNIFORM,
+            ]
         )
         if has_scale_prior.any():
             warnings.warn(
@@ -779,16 +783,13 @@ def _reparametrize_log_scaled(
             # A prior over the unscaled parameter. Its conversion already
             #  preserves the objective function.
             continue
-        if (
-            row.get(v1.C.LOWER_BOUND, 1) <= 0
-            or row.get(v1.C.UPPER_BOUND, 1) <= 0
+        if int(row[v1.C.ESTIMATE]) and (
+            row[v1.C.LOWER_BOUND] <= 0 or row[v1.C.UPPER_BOUND] <= 0
         ):
             # v1 allows a lower bound of 0 for log-scaled parameters.
             #  This would be -inf on log scale, where neither the v1 default
             #  prior nor a parameterScaleUniform prior are proper.
-            if int(row[v1.C.ESTIMATE]) and (
-                reparametrize == "all" or prior_type is not None
-            ):
+            if reparametrize == "all" or prior_type is not None:
                 zero_bound.append(par_id)
             continue
 
@@ -834,7 +835,7 @@ def _reparametrize_log_scaled(
         warnings.warn(
             f"Log-scaled parameters {', '.join(zero_bound)} were not "
             "reparametrized, because their bounds are not positive. "
-            "They are estimated on linear scale, and their parameter-scale "
+            "They are estimated on linear scale, and any parameter-scale "
             "priors were converted to the corresponding priors of the "
             "unscaled parameters, which changes the maximum a posteriori "
             "estimate.",
@@ -847,8 +848,8 @@ def _reparametrize_log_scaled(
     #  drop selected parameters that don't have parameter-scale priors.
     #  Otherwise, `reparametrize_log` will raise.
     slots = _get_override_slots(problem).values()
+    excluded = set()
     for base, base_selected in selected.items():
-        excluded = set()
         changed = True
         while changed:
             changed = False
@@ -866,22 +867,29 @@ def _reparametrize_log_scaled(
                         base_selected -= optional
                         excluded |= optional
                         changed = True
-        if excluded:
-            warnings.warn(
-                "Log-scaled parameters "
-                f"{', '.join(sorted(excluded))} were not reparametrized, "
-                "because they are used as overrides for placeholders that "
-                "cannot be log-transformed.",
-                stacklevel=4,
-            )
+    if excluded:
+        warnings.warn(
+            "Log-scaled parameters "
+            f"{', '.join(sorted(excluded))} were not reparametrized, "
+            "because they are used as overrides for placeholders that "
+            "cannot be log-transformed.",
+            stacklevel=4,
+        )
 
     for base, base_selected in selected.items():
-        reparametrize_log(
-            problem,
-            # keep the parameter table order
-            [p for p in candidates[base] if p in base_selected],
-            base=base,
-            priors={p: candidates[base][p] for p in base_selected},
-        )
+        try:
+            reparametrize_log(
+                problem,
+                # keep the parameter table order
+                [p for p in candidates[base] if p in base_selected],
+                base=base,
+                priors={p: candidates[base][p] for p in base_selected},
+            )
+        except NotImplementedError as e:
+            raise NotImplementedError(
+                "Log-scaled parameters could not be reparametrized. "
+                "Use `reparametrize='none'` to estimate them on linear "
+                "scale.\n" + str(e)
+            ) from e
 
     return any(selected.values())

petab/v2/reparametrize.py

@@ -92,9 +92,9 @@ def reparametrize_log(
     :return:
         A dictionary mapping old to new parameter IDs.
     :raises ValueError:
-        If a parameter cannot be reparametrized, e.g., because its lower bound
-        is not positive, or if its prior cannot be expressed in terms of the
-        new parameter.
+        If a parameter cannot be reparametrized, e.g., because it is estimated
+        and its lower bound is not positive, or if its prior cannot be
+        expressed in terms of the new parameter.
     :raises NotImplementedError:
         If the problem has features that are not supported yet,
         e.g., multiple or non-SBML models, mapping table entries for the
@@ -152,8 +152,14 @@ def reparametrize_log(
                 raise NotImplementedError(
                     f"Model entity `{p}' is not an SBML parameter."
                 )
+            sbml_model = model.sbml_model
+            if (sbml_model.getLevel(), sbml_model.getVersion()) < (2, 2):
+                raise NotImplementedError(
+                    "Reparametrization of model parameters requires "
+                    "initial assignments (SBML L2V2 or later)."
+                )
             model_entities.append(p)
-        if any(bound is not None and bound <= 0 for bound in (par.lb, par.ub)):
+        if par.estimate and (par.lb <= 0 or par.ub <= 0):
             # (bounds are required for estimated parameters)
             raise ValueError(
                 f"Bounds of parameter `{p}' must be positive, "
@@ -199,8 +205,9 @@ def reparametrize_log(
         new_parameters[p] = Parameter(
             id=p,
             name=par.name and f"{prefix.removesuffix('_')}({par.name})",
-            lb=None if par.lb is None else log_b(par.lb),
-            ub=None if par.ub is None else log_b(par.ub),
+            # bounds of non-estimated parameters are irrelevant
+            lb=log_b(par.lb) if par.lb is not None and par.lb > 0 else None,
+            ub=log_b(par.ub) if par.ub is not None and par.ub > 0 else None,
             nominal_value=nominal_value,
             estimate=par.estimate,
             prior_distribution=prior_distribution,
@@ -380,8 +387,7 @@ def quote(values) -> str:
         if others:
             error += (
                 f"\n  - parameters that are not reparametrized: "
-                f"{quote(others)} (consider including them in "
-                "`parameter_ids')"
+                f"{quote(others)}"
             )
         if non_positive:
             error += "\n  - non-positive numbers: " + ", ".join(
@@ -396,8 +402,9 @@ def quote(values) -> str:
             "all overrides of a placeholder to be reparametrized parameters "
             "or positive numbers.\n"
             + "\n".join(errors)
-            + "\nSplitting observables is not supported. Exclude the "
-            "affected parameters or split the observables manually."
+            + "\nSplitting observables is not supported. Reparametrize "
+            "all or none of the parameters overriding a placeholder, "
+            "or split the observables manually."
         )
 
     if not slots_to_transform:

tests/v2/test_conversion.py

@@ -363,6 +363,87 @@ def test_petab1to2_reparametrize_priors(
     np.testing.assert_allclose(v2_pdf, v1_pdf, rtol=1e-12)
 
 
+@pytest.mark.filterwarnings(
+    "ignore:.*Parameter scales are not supported in PEtab v2.*:UserWarning"
+)
+def test_petab1to2_reparametrize_llh(tmp_path):
+    """Test that reparametrizing overrides and parameters used in formulas
+    preserves the likelihood, for both bases."""
+
+    def par(par_id, scale, nominal_value):
+        return {
+            v1.C.PARAMETER_ID: par_id,
+            v1.C.PARAMETER_SCALE: scale,
+            v1.C.LOWER_BOUND: 1e-3,
+            v1.C.UPPER_BOUND: 1e3,
+            v1.C.NOMINAL_VALUE: nominal_value,
+            v1.C.ESTIMATE: 1,
+            v1.C.OBJECTIVE_PRIOR_TYPE: v1.C.PARAMETER_SCALE_NORMAL,
+            v1.C.OBJECTIVE_PRIOR_PARAMETERS: "0;1",
+        }
+
+    yaml_file = _write_v1_problem(
+        tmp_path,
+        observables=[
+            {
+                v1.C.OBSERVABLE_ID: "obs_B",
+                v1.C.OBSERVABLE_FORMULA: "observableParameter1_obs_B * B "
+                "+ offset",
+                v1.C.NOISE_FORMULA: "noiseParameter1_obs_B",
+            }
+        ],
+        measurements=[
+            {
+                v1.C.OBSERVABLE_ID: "obs_B",
+                v1.C.SIMULATION_CONDITION_ID: "c0",
+                v1.C.TIME: float(t),
+                v1.C.MEASUREMENT: 0.3 + t,
+                v1.C.OBSERVABLE_PARAMETERS: obs_par,
+                v1.C.NOISE_PARAMETERS: noise_par,
+            }
+            for t, (obs_par, noise_par) in enumerate(
+                [("s1", "sd"), ("s2", "0.2"), ("4", "sd")]
+            )
+        ],
+        parameters=[
+            _K1,
+            par("s1", v1.C.LOG10, 3.0),
+            par("s2", v1.C.LOG10, 0.7),
+            par("sd", v1.C.LOG, 0.25),
+            par("offset", v1.C.LOG10, 0.13),
+        ],
+    )
+    v1_problem = v1.Problem.from_yaml(yaml_file)
+    v2_problem = petab1to2(yaml_file, reparametrize="priors")
+    assert [p.id for p in v2_problem.parameters] == [
+        "k1",
+        "log10_s1",
+        "log10_s2",
+        "log_sd",
+        "log10_offset",
+    ]
+
+    def simulate(measurement_df):
+        return measurement_df.rename(
+            columns={v1.C.MEASUREMENT: v1.C.SIMULATION}
+        ).assign(**{v1.C.SIMULATION: [0.5, 0.6, 0.7]})
+
+    llh_v1 = calculate_llh_v1(
+        v1_problem.measurement_df,
+        simulate(v1_problem.measurement_df),
+        v1_problem.observable_df,
+        v1_problem.parameter_df,
+    )
+    llh_v2 = calculate_llh_v2(
+        v2_problem.measurement_df,
+        simulate(v2_problem.measurement_df),
+        v2_problem.observable_df,
+        v2_problem.parameter_df,
+    )
+    assert np.isfinite(llh_v1)
+    assert llh_v2 == pytest.approx(llh_v1, rel=1e-12)
+
+
 @pytest.mark.filterwarnings(
     "ignore:.*Parameter scales are not supported in PEtab v2.*:UserWarning",
     "ignore:Initialisation priors.*:UserWarning",
@@ -568,10 +649,11 @@ def test_petab1to2_reparametrize_overrides_unresolvable(tmp_path):
         ],
         overrides=["s1", "s2"],
     )
-    with pytest.raises(
-        NotImplementedError, match=r"not reparametrized: `s2' \("
-    ):
+    with pytest.raises(NotImplementedError) as excinfo:
         petab1to2(yaml_file, reparametrize="priors")
+    msg = str(excinfo.value)
+    assert "Use `reparametrize='none'`" in msg
+    assert "not reparametrized: `s2'\n" in msg
 
 
 @pytest.mark.filterwarnings(
@@ -618,11 +700,19 @@ def test_petab1to2_reparametrize_output_parameter(tmp_path):
     assert problem["obs_B"].formula == sympify_petab("10^log10_scaling * B")
 
 
-def test_petab1to2_reparametrize_warnings_and_errors(tmp_path):
+@pytest.mark.parametrize(
+    "prior_type",
+    [
+        v1.C.PARAMETER_SCALE_NORMAL,
+        v1.C.PARAMETER_SCALE_LAPLACE,
+        v1.C.PARAMETER_SCALE_UNIFORM,
+    ],
+)
+def test_petab1to2_reparametrize_warnings_and_errors(tmp_path, prior_type):
     k1 = _K1 | {
         v1.C.PARAMETER_SCALE: v1.C.LOG10,
-        v1.C.OBJECTIVE_PRIOR_TYPE: v1.C.PARAMETER_SCALE_NORMAL,
-        v1.C.OBJECTIVE_PRIOR_PARAMETERS: "0;1",
+        v1.C.OBJECTIVE_PRIOR_TYPE: prior_type,
+        v1.C.OBJECTIVE_PRIOR_PARAMETERS: "-1;1",
     }
     yaml_file = _write_v1_problem(
         tmp_path,
@@ -1034,8 +1124,15 @@ def test_petab1to2_validate(tmp_path):
 @pytest.mark.filterwarnings(
     "ignore:.*Parameter-scale priors of log-scaled parameters.*:UserWarning"
 )
+@pytest.mark.filterwarnings(
+    "ignore:Log-scaled parameters .* were not reparametrized.*:UserWarning"
+)
+@pytest.mark.filterwarnings(
+    "ignore:Parameter .* has no positive nominal value.*:UserWarning"
+)
+@pytest.mark.parametrize("reparametrize", ["none", "all"])
 @parametrize_or_skip
-def test_benchmark_collection(problem_id):
+def test_benchmark_collection(problem_id, reparametrize):
     """Test that we can upgrade all benchmark collection models."""
     logging.basicConfig(level=logging.DEBUG)
 
@@ -1045,8 +1142,10 @@ def test_benchmark_collection(problem_id):
 
     yaml_path = benchmark_models_petab.get_problem_yaml_path(problem_id)
     try:
-        problem = petab1to2(yaml_path)
+        problem = petab1to2(yaml_path, reparametrize=reparametrize)
     except NotImplementedError as e:
+        if "could not be reparametrized" in str(e):
+            raise
         pytest.skip(str(e))
     assert isinstance(problem, Problem)
     assert len(problem.measurements)

tests/v2/test_reparametrize.py

@@ -234,6 +234,24 @@ def test_reparametrize_log_invalid():
     with pytest.raises(NotImplementedError, match="mapping table"):
         reparametrize_log(problem, ["scaling"])
 
+    problem = _problem()
+    doc = problem.model.sbml_model.getSBMLDocument()
+    assert doc.setLevelAndVersion(2, 1)
+    with pytest.raises(NotImplementedError, match="L2V2"):
+        reparametrize_log(problem, ["k1"])
+
+
+def test_reparametrize_log_fixed_nonpositive_bounds():
+    """Bounds of non-estimated parameters are irrelevant."""
+    problem = _problem()
+    problem["offset2"].lb = 0
+    problem["offset2"].ub = 10
+    reparametrize_log(problem, ["offset1", "offset2"])
+    par = problem["log10_offset2"]
+    assert par.lb is None
+    assert par.ub == 1
+    assert par.nominal_value == pytest.approx(math.log10(6))
+
 
 def test_reparametrize_log_unresolvable_override():
     # offset1 is reparametrized, offset2 is not
@@ -248,7 +266,7 @@ def test_reparametrize_log_unresolvable_override():
         "Observable placeholder `offset_A' of observable `obs_A' "
         "(overridden by `offset1') also receives:" in msg
     )
-    assert "not reparametrized: `offset2' (consider including" in msg
+    assert "not reparametrized: `offset2'\n" in msg
     assert "non-positive" not in msg
 
     # non-positive numeric overrides, in several placeholders, reported at