From bf7dc3bff1327da186529ab8420ac6bd7d60c7e0 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Thu, 17 Sep 2026 14:21:49 -0600 Subject: [PATCH] fix(petab): refuse a time_error fit on export instead of writing it as an exact-time one (#738) A `time_error` clause on a `noise_model` line (ADR-0112, #587) says the reported measurement times are not exact: the objective integrates each observation's density over a prior on its true sampling time. `ploop` stores it under its own structural key, `('time_error', observable)`, and `config.py` reads that key to swap the whole per-point objective for a `MarginalizedTimeObjective`. The exporter had no awareness of the clause whatsoever -- `grep -n time_error pybnf/petab/export.py` was empty -- so a job carrying it exported with no warning and no exception, and the emitted problem held the measurements at their nominal times as though those were exact. Measured end to end on the demo job: `MarginalizedTimeObjective` before the round trip, `LikelihoodObjective` after, with the re-imported conf containing no `time_error` substring at all. That is a different statistical model, reached silently. Its sibling in the very same `noise_model` field grammar was guarded all along. `cumulative` is stored under the same kind of key by the same `nm_field` alternation, and `_reject_cumulative` refuses it with reasoning that transfers verbatim: exporting "would silently drop it and emit a problem that scores the raw cumulative columns, a different objective. Refuse instead (the project's fail-loud-over-silently-wrong stance)". That function's twin for `time_error` was never written. It is now, beside it, called from `export_job` between `_reject_cumulative` and `_reject_normalization`. Like the `u` flag of #736 and unlike #719 and #733, this is a real PEtab boundary rather than an unwritten mapping: a `measurements` row carries one exact `time` and has no field for a distribution over it. So the fix is a refusal naming the boundary, not a richer export. The refusal keys on the `('time_error', observable)` key rather than on the whole-fit shape that is reachable today, because the exporter reads the raw config while `_maybe_marginalize_time` is what defers the per-observable form; it names the observables when it finds them and says the fit when the clause is whole-fit. The `sigma_t` scale source rides on the same key and goes with it. Walking the whole `nm_field` alternation, `time_error`/`sigma_t` was the last gap of this shape in the objective/noise surface: `cumulative` and a mean-centred `location` were already refused, and `prediction_formula`, `formula` and the noise source all export and round-trip under test (the `predsigma_v2` and `scaling_v2` fixtures). Tests: three, of which two fail against the old code. The whole-fit clause is refused; a per-observable clause is refused too, which pins that the guard reads the config key rather than the currently-reachable shape; and a control exports the same fixture with the time clause removed, so the refusal cannot pass for some other property of the conf. The boundary is added to the two lists that enumerate them: the module docstring's in-code list, and the PEtab page, which now names the two refusals a user is most likely to meet on a job that otherwise looks perfectly exportable. Signed-off-by: Bill Hlavacek --- CHANGELOG.md | 17 ++++++++++++++++ docs/petab.rst | 12 +++++++---- pybnf/petab/export.py | 41 +++++++++++++++++++++++++++++++++++++- tests/test_petab_export.py | 40 +++++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 065b6820..f31795ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -252,6 +252,23 @@ All notable changes to PyBNF are documented below. This project adheres to by default. Both surfaces are documented under gradient-based fitting. ### Fixed +- **A `time_error` fit is refused by the PEtab export instead of being written as an + exact-time one (#738).** A `time_error` clause on a `noise_model` line says the reported + measurement times are not exact: the objective integrates each observation's density over a + prior on its true sampling time, and loading the config swaps the whole per-point objective + for a `MarginalizedTimeObjective`. The exporter had no awareness of the clause at all, so + such a job exported without a warning and the emitted problem carried the measurements at + their nominal times as though exact. Round-tripped, the fit came back scored by + `LikelihoodObjective` — a different statistical model, silently. + Its sibling in the very same `noise_model` grammar was guarded all along: `cumulative` is + stored under the same kind of structural key and refused by `_reject_cumulative`, for exactly + this reason. That function's twin for `time_error` was never written; it is now, beside it. + PEtab cannot express the clause — a measurements row carries one exact `time` and has no + field for a distribution over it — so, as with the `U` tag, the fix is a refusal naming the + boundary rather than a richer export. Walking the whole noise-model field grammar, + `time_error`/`sigma_t` was the last of its shape: `cumulative` and a mean-centred `location` + were already refused, and the prediction formula, measurement formula and noise source all + export and round-trip under test. - **A `U`-tagged free parameter is refused by the PEtab export instead of being written as a hard-bounded one (#736, ADR-0025).** `uniform_var = v 0 10 U` and its `loguniform_var` twin mean the box is enforced only during initialization: it seeds the first population and the diff --git a/docs/petab.rst b/docs/petab.rst index 9f7f1015..3685ed61 100644 --- a/docs/petab.rst +++ b/docs/petab.rst @@ -68,10 +68,14 @@ free parameters, priors, noise models, and data. Both free-parameter spellings a read — the positional ``_var`` line and the edition-2 ``parameter:`` record — so a truncated prior, which only the record can state, exports and round-trips like any other. Anything PEtab v2 cannot express raises ``NotImplementedError`` naming the -boundary; nothing is dropped quietly. One boundary worth knowing before you export: a -``U``-tagged ``uniform_var``/``loguniform_var``, whose box constrains only the initial -draw, has no PEtab spelling — its bounds are hard — so that job is refused rather than -silently constrained. +boundary; nothing is dropped quietly. Two worth knowing before you export, because both +concern a job that otherwise looks perfectly exportable: + +- a ``U``-tagged ``uniform_var``/``loguniform_var``, whose box constrains only the initial + draw. PEtab's bounds are hard, so the job is refused rather than silently constrained. +- a ``time_error`` clause, which integrates each observation over a prior on its true + sampling time. A PEtab measurement carries one exact ``time``, so the job is refused + rather than silently exported as an exact-time fit. .. _petab_bngl_loader: diff --git a/pybnf/petab/export.py b/pybnf/petab/export.py index e38ac744..0337506d 100644 --- a/pybnf/petab/export.py +++ b/pybnf/petab/export.py @@ -55,7 +55,10 @@ (BNGL + SBML may mix). Everything else raises ``NotImplementedError`` (the boundary is in code, not silent): an objective PEtab cannot represent (``neg_bin*`` -- removed from v2; ``lognormal`` -- log10 vs PEtab natural log; a free-parameter or relative sigma; -``direct_pass``/``kl``/``wasserstein``); the no-prior ``var``/``logvar``; a ``.con``/``.prop`` +``direct_pass``/``kl``/``wasserstein``); the no-prior ``var``/``logvar``; a ``u``-flagged +Uniform, whose box seeds the draw without constraining the search (#736); a ``time_error`` +measurement-time marginalization, whose latent sampling time a PEtab measurement row's single +exact ``time`` cannot carry (#738); a ``.con``/``.prop`` Constraint; an Antimony (``.ant``) model. The oracle is petab's full ``default_validation_tasks`` via ``Problem.from_yaml`` + the native ``BnglModel`` loader (ADR-0026), wired into the tests; see ADR-0025/0027/0028/0036/0040. @@ -183,6 +186,7 @@ def export_job(conf_path, out_dir, inline_functions=False): noise = _resolve_noise(conf) per_obs_noise = _resolve_per_observable_noise(conf) _reject_cumulative(conf) + _reject_time_error(conf) _reject_normalization(conf) free_params = _free_parameters_from_conf(conf) # An estimated (`fit`) sigma exports as a bare-id noiseFormula naming an estimated PEtab @@ -932,6 +936,41 @@ def _reject_cumulative(conf): f"to per-interval increments yourself) to export to PEtab.") +def _reject_time_error(conf): + """Fail loud if the job marginalizes the latent measurement time (ADR-0112, #587). + + A ``time_error`` clause on a ``noise_model`` line says the reported times are **not + exact**: the objective integrates each observation's density over a prior on its true + sampling time, and ``config.py`` swaps the whole per-point objective for a + ``MarginalizedTimeObjective`` (a fit that reads ``MarginalizedTimeObjective`` before an + export reads ``LikelihoodObjective`` after one). A PEtab ``measurements`` row carries one + exact ``time`` and has no field for a distribution over it, so there is nothing to write + the clause as: exporting emits a problem scored at the nominal reported times by the + ordinary likelihood, which is a different statistical model. + + Refuse instead -- the same stance and the same reason as :func:`_reject_cumulative`, whose + clause is a sibling in the very same ``noise_model`` grammar and was already guarded here + while this one was not (#738). The ``sigma_t`` scale source rides on the same key, so it + goes with it. + """ + keys = [k for k in conf if isinstance(k, tuple) and k[0] == 'time_error'] + if not keys: + return + # The observable is None for the whole-fit form, which is the only one a job can + # currently run (``_maybe_marginalize_time`` defers per-observable time priors), but the + # exporter reads the raw config, so name whichever shape is actually present. + observables = sorted(k[1] for k in keys if k[1] is not None) + subject = (f"Observable(s) {observables} declare" if observables + else "This fit declares") + raise NotImplementedError( + f"{subject} a 'time_error' measurement-time marginalization (ADR-0112, #587), which " + f"PEtab v2 cannot express -- a measurements row carries one exact 'time' and has no " + f"field for a distribution over it. Exporting would emit a problem scored at the " + f"nominal reported times by the ordinary likelihood, a different objective. Remove " + f"the 'time_error' and 'sigma_t' fields from the noise_model line to export to " + f"PEtab.") + + def _reject_normalization(conf): """Fail loud if the job declares any normalization (ADR-0053, #444; floor/scale ADR-0066, #479). diff --git a/tests/test_petab_export.py b/tests/test_petab_export.py index c6ec9862..743cf5f3 100644 --- a/tests/test_petab_export.py +++ b/tests/test_petab_export.py @@ -2280,6 +2280,46 @@ def test_mean_centered_noise_model_not_implemented(self, tmp_path): "uniform_var = v1 0 10\n"), tmp_path / 'out') + def test_time_error_marginalization_not_implemented(self, tmp_path): + # A time_error clause (ADR-0112, #587) says the reported times are not exact: the + # objective integrates each observation over a prior on its true sampling time, and + # config.py swaps the whole objective for a MarginalizedTimeObjective. A PEtab + # measurements row carries one exact time, so there is nothing to write it as -- + # exporting emitted a problem scored at the nominal times by the ordinary likelihood, + # a different statistical model, and said nothing (#738). Its sibling in the same + # noise_model grammar, `cumulative`, was guarded here all along. + with pytest.raises(NotImplementedError, match='time_error'): + export_job(_boundary_conf( + tmp_path, + "noise_model = gaussian, sigma = fix_at 1, " + "time_error = truncated_normal, sigma_t = fix_at 0.5\n" + "uniform_var = v1 0 10\n"), + tmp_path / 'out') + + def test_per_observable_time_error_is_also_refused(self, tmp_path): + # The whole-fit form is the only one a job can currently run (_maybe_marginalize_time + # defers per-observable time priors), but the exporter reads the RAW config, so the + # refusal keys on the ('time_error', observable) key rather than on the shape that + # happens to be reachable today -- and names the observable it found. + with pytest.raises(NotImplementedError, match=r"time_error|\['x'\]"): + export_job(_boundary_conf( + tmp_path, + "noise_model = gaussian, sigma = fix_at 1\n" + "noise_model x = gaussian, sigma = fix_at 2, " + "time_error = uniform, sigma_t = fix_at 0.5\n" + "uniform_var = v1 0 10\n"), + tmp_path / 'out') + + def test_the_same_job_without_the_time_clause_exports(self, tmp_path): + # The control: everything else about the conf is exportable, so the refusal above is + # the time clause and not some other property of the fixture. + export_job(_boundary_conf( + tmp_path, + "noise_model = gaussian, sigma = fix_at 1\n" + "uniform_var = v1 0 10\n"), + tmp_path / 'out') + assert (tmp_path / 'out' / 'parameters.tsv').is_file() + def test_cumulative_prediction_transform_not_implemented(self, tmp_path): # The cumulative->incident differencing (ADR-0051, #418) is a PyBNF prediction # transform with no PEtab v2 representation: refuse rather than silently export a