fix(petab): refuse a time_error fit on export instead of writing it as an exact-time one (#738)

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

docs/petab.rst

@@ -68,10 +68,14 @@ free parameters, priors, noise models, and data. Both free-parameter spellings a
 read — the positional ``<family>_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:
 

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).
 

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