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