diff --git a/docs/adr/0073-petab-v1-log10-observabletransformation-is-re-injected-as-a-preserved-column-and-imported-as-the-native-lognormal-family.md b/docs/adr/0073-petab-v1-log10-observabletransformation-is-re-injected-as-a-preserved-column-and-imported-as-the-native-lognormal-family.md index fbe031d8..7b3614ee 100644 --- a/docs/adr/0073-petab-v1-log10-observabletransformation-is-re-injected-as-a-preserved-column-and-imported-as-the-native-lognormal-family.md +++ b/docs/adr/0073-petab-v1-log10-observabletransformation-is-re-injected-as-a-preserved-column-and-imported-as-the-native-lognormal-family.md @@ -118,3 +118,35 @@ log10, PEtab `log-normal` is natural log"), **0032** (the importer read path tha `noiseDistribution`), **0011** (the per-point kernel and the median location). Sibling: **#491** (`petab1to2_preserve_scale` re-injecting `parameterScale` as a `log-uniform` prior — the parameter-axis twin). Related: the benchmark build siblings (#492–#496). + +## Addendum (2026-09-10): the converter also resets `noiseDistribution` to the v1 base family (issue #679) + +**Accepted and implemented 2026-09-10.** The re-injection above was silently leaning on a +petab bug. `petab1to2` is *designed* to fold the v1 `observableTransformation` into the v2 +`noiseDistribution`; a missing `return` in that merge left the column blank in every +petab < 0.9.0, the importer defaulted the blank to `normal`, and `log10` over `normal` +imported as `lognormal` exactly as this ADR describes. petab 0.9.0 (2026-09-07, +PEtab-dev/libpetab-python#502) fixed the merge. Because PEtab v2 has no `log10-normal`, the +converter now substitutes the natural-log family — `log10` + `normal` → `log-normal`, with a +warning — which is a silent `ln 10` rescaling of sigma. Stacked under our re-injected `log10` +column, the importer's "give the residual scale in one place" rule refused every converted +log10 observable as a contradiction, and `test_converted_log10_problem_imports_as_lognormal` +went red on every Python ≥ 3.12 CI leg (petab 0.9.0 requires 3.12, so 3.11 stayed on 0.8.2 +and green). + +The fix keeps the ADR's design and closes the gap it left: `petab1to2_preserve_scale` now +reads each log observable's **v1** `noiseDistribution` (blank → `normal`) alongside its +transformation, and `inject_observable_transformations` takes an optional +`{observableId: 'normal' | 'laplace'}` map and resets the row's `noiseDistribution` to that +linear base while writing the transformation. The scale is then stated once, in the preserved +column, regardless of which petab produced the v2 table — a blank cell (0.8.2) and a folded +`log-normal` / `log-laplace` (0.9.0) both come back to the v1 family. Rows without a +transformation keep whatever petab1to2 wrote, so a linear problem is still byte-identical. +petab's own substitution warning is inside the `catch_warnings` block the converter already +uses to silence the `parameterScale` warning, for the same reason: re-adding the scale is +this function's job. + +This is not an upstream bug. The substitution is documented and warned, and the underlying +gap — no log10-normal in PEtab v2 — is a specification decision (ADR-0022's "PyBNF +`lognormal` is log10, PEtab `log-normal` is natural log"). The preserved column is the +workaround the specification leaves us. diff --git a/pybnf/petab/convert.py b/pybnf/petab/convert.py index 112f7b62..7bcdf54e 100644 --- a/pybnf/petab/convert.py +++ b/pybnf/petab/convert.py @@ -27,15 +27,20 @@ change-of-variables Jacobian) -- a *different objective* from the linear residual, not just a different search. PEtab v2 removed the column and folded transformation into ``noiseDistribution`` as **natural-log** ``log-normal`` / ``log-laplace`` prefixes, with **no -log10 form**; ``petab1to2`` therefore drops a v1 ``log10`` transformation entirely (it -downgrades ``log10-normal`` to a blank ``noiseDistribution``), so the observable imports as a -linear Gaussian and the fit optimises the wrong objective. Since v2 has no faithful +log10 form**; ``petab1to2`` therefore cannot carry a v1 ``log10`` transformation faithfully. +petab < 0.9.0 dropped it entirely (a missing ``return`` left ``noiseDistribution`` blank, so +the observable imported as a linear Gaussian and the fit optimised the wrong objective); +petab >= 0.9.0 substitutes the natural-log family (``log10-normal`` -> ``log-normal``, with a +warning), which silently rescales sigma by ``ln 10``. Since v2 has no faithful representation, we re-inject ``observableTransformation`` as a **preserved extra column** on -the v2 observables table: PyBNF's importer reads it to select the noise family's additive -scale (``lin`` / ``log10`` / ``log``; :mod:`pybnf.petab.observables`, -:mod:`pybnf.petab.import_`), and other PEtab v2 tools ignore the unknown column (it passes v2 -lint). This is directly parallel to the parameterScale re-injection above -- the same -"re-add the scale petab1to2 dropped" migration, on the observable axis. +the v2 observables table and reset ``noiseDistribution`` to the v1 **linear base family** +(``normal`` / ``laplace``), so the residual scale is stated in exactly one place: PyBNF's +importer reads the column to select the noise family's additive scale (``lin`` / ``log10`` / +``log``; :mod:`pybnf.petab.observables`, :mod:`pybnf.petab.import_`) and refuses a log +transformation over an already-log distribution as a contradiction (issue #679), while other +PEtab v2 tools ignore the unknown column (it passes v2 lint). This is directly parallel to +the parameterScale re-injection above -- the same "re-add the scale petab1to2 dropped" +migration, on the observable axis. This is the migration ``petab1to2`` should offer as an opt-in; it lives here as an explicit, named converter so :func:`pybnf.petab.import_job` stays a pure v2 importer with @@ -61,6 +66,11 @@ #: v1 spelling; PyBNF's importer reads it, other v2 tools ignore it). _OBSERVABLE_TRANSFORMATION_COLUMN = 'observableTransformation' +#: The v2 observables column petab1to2 folds the v1 transformation into (as the natural-log +#: family, petab >= 0.9.0; blank before). Reset to the v1 linear base family alongside the +#: re-injected transformation so the scale is stated once (issue #679). +_NOISE_DISTRIBUTION_COLUMN = 'noiseDistribution' + def petab1to2_preserve_scale(v1_yaml_path, out_dir): """Convert a PEtab **v1** problem to **v2**, preserving log estimation scales. @@ -123,12 +133,18 @@ def petab1to2_preserve_scale(v1_yaml_path, out_dir): # 4. Re-inject the dropped observableTransformation as a preserved column (issue #499). # v2 has no log10 noiseDistribution, so a log/log10 observable has no v2-native home; # the importer reads this extra column to pick the noise family's additive scale. - transformations = _v1_observable_transformations(v1_yaml_path, v1_spec) - if transformations: + # petab1to2 folded the same transformation into noiseDistribution as the natural-log + # family (petab >= 0.9.0; blank before), so that column is reset to the v1 linear base + # (normal / laplace) -- the importer refuses a log transformation over an already-log + # distribution as a contradiction (issue #679). + scales = _v1_observable_scales(v1_yaml_path, v1_spec) + if scales: + transformations = {oid: transformation for oid, (transformation, _) in scales.items()} + distributions = {oid: distribution for oid, (_, distribution) in scales.items()} for obs_file in v2_spec.get('observable_files', []): v2_obs_path = out_dir / obs_file v2_odf = pd.read_csv(v2_obs_path, sep='\t') - inject_observable_transformations(v2_odf, transformations) + inject_observable_transformations(v2_odf, transformations, distributions) v2_odf.to_csv(v2_obs_path, sep='\t', index=False) return v2_yaml @@ -185,7 +201,7 @@ def inject_log_uniform_priors(v2_pdf, log_estimated_ids, declared_prior_ids=None return v2_pdf -def inject_observable_transformations(v2_odf, transformations): +def inject_observable_transformations(v2_odf, transformations, distributions=None): """Add an ``observableTransformation`` column to a v2 observables DataFrame in place. ``transformations`` is a ``{observableId: 'log' | 'log10'}`` map (linear observables are @@ -194,6 +210,15 @@ def inject_observable_transformations(v2_odf, transformations): PEtab v2 dropped the column, so this is a **preserved extra column** the importer reads to select the noise family's additive scale (:mod:`pybnf.petab.observables`); other v2 tools ignore it. Mutates and returns ``v2_odf`` (a v2 observables :class:`pandas.DataFrame`). + + ``distributions``, when given, is the matching ``{observableId: 'normal' | 'laplace'}`` + map of each log observable's **v1** ``noiseDistribution`` -- its linear base family. Every + row that receives a transformation also has its ``noiseDistribution`` reset to that base. + petab1to2 folds the v1 transformation into this column as the natural-log family + (``log10-normal`` -> ``log-normal``, petab >= 0.9.0; a blank cell before that), and the + importer refuses a log transformation stacked over an already-log distribution, so the + scale must be stated in the transformation column alone (issue #679). Rows not in the + map keep whatever distribution petab1to2 wrote. """ col = _OBSERVABLE_TRANSFORMATION_COLUMN if col not in v2_odf.columns: @@ -201,25 +226,38 @@ def inject_observable_transformations(v2_odf, transformations): # petab1to2 may emit an all-empty column as float64 (NaN); coerce to object so the string # cells below don't raise a dtype error. Untouched rows still write as blank. v2_odf[col] = v2_odf[col].astype('object') + dcol = _NOISE_DISTRIBUTION_COLUMN + if distributions: + if dcol not in v2_odf.columns: + v2_odf[dcol] = '' + v2_odf[dcol] = v2_odf[dcol].astype('object') for i, row in v2_odf.iterrows(): - transformation = transformations.get(str(row['observableId'])) + oid = str(row['observableId']) + transformation = transformations.get(oid) if transformation is not None: v2_odf.at[i, col] = transformation + if distributions and oid in distributions: + v2_odf.at[i, dcol] = distributions[oid] return v2_odf -def _v1_observable_transformations(v1_yaml_path, v1_spec): - """``{observableId: 'log' | 'log10'}`` for every v1 observable with a log residual scale. +def _v1_observable_scales(v1_yaml_path, v1_spec): + """``{observableId: ('log' | 'log10', 'normal' | 'laplace')}`` for every v1 observable + with a log residual scale: its transformation and the linear base family of its v1 + ``noiseDistribution`` (blank -> ``normal``, the v1 default). Reads each v1 problem's observable file(s) and keeps only the ``log`` / ``log10`` transformations (linear -- or an absent column -- is the v2 default and needs no - re-injection). ``petab.v2.petab1to2`` drops this column, so the scale-preserving converter - re-injects what this returns (issue #499), mirroring the parameterScale re-injection. + re-injection). ``petab.v2.petab1to2`` drops the transformation column and folds it into + ``noiseDistribution``, so the scale-preserving converter re-injects the transformation and + restores the base distribution from what this returns (issues #499, #679), mirroring the + parameterScale re-injection. """ + import pandas as pd import petab.v1 as petab_v1 import petab.v1.C as C1 - transformations = {} + scales = {} for problem in v1_spec.get('problems', []): for obs_file in problem.get('observable_files', []): odf = petab_v1.get_observable_df(str(Path(v1_yaml_path).parent / obs_file)) @@ -228,9 +266,13 @@ def _v1_observable_transformations(v1_yaml_path, v1_spec): for oid, row in odf.iterrows(): value = row.get(C1.OBSERVABLE_TRANSFORMATION) transformation = str(value if value is not None else C1.LIN).strip().lower() - if transformation in _LOG_TRANSFORMATIONS: - transformations[str(oid)] = transformation - return transformations + if transformation not in _LOG_TRANSFORMATIONS: + continue + dist = row.get(C1.NOISE_DISTRIBUTION) + if dist is None or pd.isna(dist) or not str(dist).strip(): + dist = C1.NORMAL + scales[str(oid)] = (transformation, str(dist).strip().lower()) + return scales def _sole_yaml(out_dir): diff --git a/tests/test_petab_convert.py b/tests/test_petab_convert.py index 82d42e5a..a9ff7b15 100644 --- a/tests/test_petab_convert.py +++ b/tests/test_petab_convert.py @@ -80,6 +80,18 @@ def test_log10_observable_transformation_is_reinjected(self, tmp_path): row = obs.set_index('observableId').loc['obs_V'] assert row['observableTransformation'] == 'log10' + def test_log10_observable_keeps_its_linear_base_distribution(self, tmp_path): + # petab1to2 folds the v1 transformation into noiseDistribution: petab < 0.9.0 left it + # blank (a missing return), petab >= 0.9.0 substitutes the natural-log family + # (log10-normal -> log-normal, with a warning). Either way the converter must state + # the scale once -- in the re-injected column -- over the v1 base family, or the + # importer refuses log10 stacked on log-normal as a contradiction (#679). + yaml = _write_v1_problem(tmp_path / 'v1', 'log10') + v2_yaml = petab1to2_preserve_scale(str(yaml), str(tmp_path / 'v2')) + obs = pd.read_csv(v2_yaml.parent / 'observables.tsv', sep='\t') + row = obs.set_index('observableId').loc['obs_V'] + assert row['noiseDistribution'] == 'normal' + def test_linear_observable_gets_no_transformation(self, tmp_path): # A lin observable needs no re-injection (lin is the v2 default) -> no column added, # so the converted problem stays byte-identical to plain petab1to2 on the observables. @@ -230,6 +242,33 @@ def test_float64_nan_column_is_coerced_not_raised(self): inject_observable_transformations(df, {'obs_V': 'log10'}) # must not raise assert df.set_index('observableId').loc['obs_V', 'observableTransformation'] == 'log10' + def test_folded_log_normal_is_reset_to_the_v1_base(self): + # The petab >= 0.9.0 shape: the transformation already folded into noiseDistribution + # as the natural-log family. With the v1 base supplied, the log observable's row is + # reset to it and the linear observable's row is left exactly as petab1to2 wrote it. + df = self._petab1to2_obs_shape() + df['noiseDistribution'] = ['log-normal', 'normal'] + inject_observable_transformations(df, {'obs_V': 'log10'}, {'obs_V': 'normal'}) + by_id = df.set_index('observableId') + assert by_id.loc['obs_V', 'noiseDistribution'] == 'normal' + assert by_id.loc['obs_V', 'observableTransformation'] == 'log10' + assert by_id.loc['obs_lin', 'noiseDistribution'] == 'normal' + + def test_folded_log_laplace_is_reset_to_laplace(self): + # The base family is the v1 author's, not always normal: log10 + laplace comes back + # from petab1to2 as log-laplace and must return to laplace under the log10 column. + df = self._petab1to2_obs_shape() + df['noiseDistribution'] = ['log-laplace', ''] + inject_observable_transformations(df, {'obs_V': 'log10'}, {'obs_V': 'laplace'}) + assert df.set_index('observableId').loc['obs_V', 'noiseDistribution'] == 'laplace' + + def test_without_distributions_the_column_is_untouched(self): + # The pre-#679 contract: callers that pass no base map get only the transformation. + df = self._petab1to2_obs_shape() + df['noiseDistribution'] = ['log-normal', ''] + inject_observable_transformations(df, {'obs_V': 'log10'}) + assert df.set_index('observableId').loc['obs_V', 'noiseDistribution'] == 'log-normal' + class TestHelpers: