From 44072426dec8993d5fc66dcbe5e3b96c3cfe02e1 Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Thu, 17 Sep 2026 13:45:18 -0600 Subject: [PATCH] fix(petab): refuse a U-tagged free parameter on export instead of writing it as a hard-bounded one (#736) `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 search is then free to leave it. `ploop` reads the tag into the value list and the config loader honours it, so the fitter builds a parameter whose box is `(-inf, inf)`. The exporter read `value[0]` and `value[1]` and stopped. The comment above the construction called the third token "the native `bounded` flag, inert for the location families" -- true there, and false where the token is actually legal, since `parse.b_var_def_keys` is exactly `['loguniform_var', 'uniform_var']`, the two families whose p1/p2 become the PEtab bounds. `bounded` therefore defaulted to True and `_petab_uniform_row` wrote `[p1, p2]` into `lowerBound`/`upperBound`, which in PEtab are hard box constraints. Measured: a conf declaring a search free to leave [0, 10] exported to a table byte-identical to the bounded spelling, and the re-imported conf built a parameter with `bounded=True` and box `(0, 10)` -- the `U` gone from the line. No warning, no exception. This is the third defect of the same shape as #719 and #733: the export reads part of a declaration and discards the rest without saying so, against docs/petab.rst's "the export is fit-preserving" and against the module's own contract that everything it cannot write raises NotImplementedError. Unlike the first two this one has no mapping waiting to be written. PEtab's nearest shape -- blank (infinite) bounds plus an explicit `priorDistribution = uniform` over the box -- says something else, because PEtab bounds truncate a prior rather than seed a draw, and it does not survive the trip either: `_resolve_prior`'s uniform arm returns `bounded=True` unconditionally, so such a row re-imports as a bounded parameter. That is the right reading for a PEtab row, whose bounds are hard, so nothing changes on the import side. So the exporter refuses. `_free_parameter_from_var_line` passes the flag through (the parameter it builds now matches the one the fitter builds), and `_petab_uniform_row` raises NotImplementedError when it is off, naming the boundary and saying to drop the tag to export the box as real bounds. Exporting the box with a warning was considered and rejected: the exporter emits no warnings anywhere, so it would be a new channel for one case, and a warning is the signal the #583/#719 work found people do not act on. The cost of refusing is nothing measurable -- no `U`-tagged declaration exists in the repo's examples, tests or benchmarks, or in the BNGL-Models/pybnf-jobs corpus. Tests: five, of which four fail against the old code. Two at the row mapper, parametrized over both flag-bearing families, which also assert the bounded spelling of the same declaration is untouched; two end to end on a real job, one per family; and one holding the other half of the flag, that an explicit `B` tag exports byte-identically to leaving it off -- the guard that threading `bounded` through the constructor perturbs nothing. ADR-0025 carries an amendment recording the refusal, the rejected warn-and-export alternative, and the evidence that the PEtab shape is genuinely absent rather than merely unmapped. The `uniform_var` config-key documentation now says the tag cannot be exported, where a reader of that tag will look. Signed-off-by: Bill Hlavacek --- CHANGELOG.md | 20 +++++++++++ ...ter-first-bngl-to-petab-petab-as-oracle.md | 25 ++++++++++++++ docs/config_keys.rst | 5 +++ docs/petab.rst | 5 ++- pybnf/petab/export.py | 12 +++++-- pybnf/petab/parameters.py | 25 ++++++++++++-- tests/test_petab_export.py | 33 +++++++++++++++++++ 7 files changed, 119 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d3b8ee72..065b68209 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -252,6 +252,26 @@ All notable changes to PyBNF are documented below. This project adheres to by default. Both surfaces are documented under gradient-based fitting. ### Fixed +- **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 + search is then free to leave it, so the parameter's box is `(-inf, inf)`. The exporter read + the two numbers and dropped the tag, and `lowerBound`/`upperBound` in PEtab are hard box + constraints — so the job exported to a table byte-identical to the bounded spelling, and a + re-import produced a fit constrained to `[0, 10]` where the original was not. No warning, no + exception. It is the third of the same shape as #719 and #733: the export read part of a + declaration and discarded the rest without saying so. + This one has no mapping waiting to be written. PEtab's nearest shape, blank bounds plus an + explicit uniform `priorDistribution` over the box, says something else — PEtab bounds + truncate a prior rather than seed a draw — and does not survive the trip either, since the + importer resolves a uniform prior to a bounded parameter whatever the bounds say. That is + correct for a PEtab row, so nothing changed on the import side. The export now refuses, in + code and naming the boundary, which is the contract it already holds itself to for every + other construct PEtab cannot state; the message says to drop the tag to export the box as + real bounds. Exporting the box with a warning was considered and rejected: the exporter + emits no warnings anywhere, and a warning is the signal the start-point work found people do + not act on. Nothing measurable is lost — no `U`-tagged declaration exists in the examples, + tests or benchmarks. The untagged and `B`-tagged spellings export exactly as before. - **The PEtab export reads edition-2 `parameter:` records, so a free parameter written the new-era way no longer vanishes from the exported problem (#733).** The exporter picked its free parameters out of the config by matching key names against `(_var$|^var$|^logvar$)`. diff --git a/docs/adr/0025-petab-interop-is-exporter-first-bngl-to-petab-petab-as-oracle.md b/docs/adr/0025-petab-interop-is-exporter-first-bngl-to-petab-petab-as-oracle.md index 0a8bf9982..98d6c1a1e 100644 --- a/docs/adr/0025-petab-interop-is-exporter-first-bngl-to-petab-petab-as-oracle.md +++ b/docs/adr/0025-petab-interop-is-exporter-first-bngl-to-petab-petab-as-oracle.md @@ -223,3 +223,28 @@ question this resolves as "test oracle"), **0023** (the observables/noise corres reversed), **0021** (the noise engine / σ-source kinds), **0004** (PEtab-defaulted, not PEtab-bound). Issue: **#407** (umbrella; re-scoped to exporter-first). Follow-ups: the `parameter_scan`/dose-response export, conditions/experiments export, then the importer. + +## Amendment (2026-09-17, #736) + +One more construct joins the "surfaced explicitly rather than silently mis-exported" list: +a Uniform family carrying the native `u` flag (`uniform_var = v 0 10 u`), which turns the +reflecting box off so that the box seeds the first population and the search is then free +to leave it. The exporter read `p1`/`p2` and dropped the flag, so such a job exported to a +table byte-identical to the bounded spelling — stating `[0, 10]` as PEtab +`lowerBound`/`upperBound`, which are hard box constraints. A search the config declared +unconstrained came back constrained, with no warning. + +**Refuse rather than warn-and-export.** The alternative — write the box as bounds and warn +— was rejected: the exporter emits no warnings anywhere, so it would be a new channel for +one case, and a warning on stderr is exactly the signal the #583/#719 work found people do +not act on. Refusing costs nothing measurable: no `u`-flagged declaration exists in the +repo's examples, tests or benchmarks, or in the `BNGL-Models/pybnf-jobs` corpus. If it ever +does become a real obstacle, warn-and-export is the fallback to revisit, with the warning +naming what changed. + +**It is a true boundary, not an unwritten mapping.** PEtab's nearest shape — blank +(infinite) bounds plus an explicit `priorDistribution = uniform` over `(0, 10)` — does not +mean the same thing and does not survive the trip either: PEtab bounds *truncate a prior* +rather than seed a draw, and `parameters._resolve_prior`'s uniform arm returns +`bounded=True` unconditionally, so that row re-imports as a bounded parameter. The importer +is right to do so — a PEtab row's bounds are hard — so nothing changes on the import side. diff --git a/docs/config_keys.rst b/docs/config_keys.rst index 143b9e004..1ca90a366 100644 --- a/docs/config_keys.rst +++ b/docs/config_keys.rst @@ -846,6 +846,11 @@ Parameter and Model Specification * ``uniform_var = k__FREE 10 20`` * ``uniform_var = k__FREE 10 20 U`` + A ``U``-tagged parameter cannot be exported to PEtab: PEtab's ``lowerBound``/``upperBound`` + are hard box constraints with no "initialization only" reading, so + :ref:`export_job ` refuses the job rather than write bounds it does not mean. + Drop the tag to export the box as real bounds. + **normal_var** A normally distributed variable defined by a 3-tuple: the name, mean value, and standard deviation. The distribution is truncated at 0 to prevent negative values diff --git a/docs/petab.rst b/docs/petab.rst index e534e5a06..9f7f1015a 100644 --- a/docs/petab.rst +++ b/docs/petab.rst @@ -68,7 +68,10 @@ 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. +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. .. _petab_bngl_loader: diff --git a/pybnf/petab/export.py b/pybnf/petab/export.py index 7d60379d8..e38ac744e 100644 --- a/pybnf/petab/export.py +++ b/pybnf/petab/export.py @@ -1456,11 +1456,17 @@ def _free_parameter_from_var_line(name, keyword, value): """The legacy positional ``_var = p1 [p2] [b|u]`` declaration.""" _require_exportable_prior(name, keyword) # p1/p2 are the family's governing values (bounds for the Uniform families, - # loc/scale or shape/scale for the two-parameter location families); a 3rd token - # is the native ``bounded`` flag, inert for the location families. A one-parameter + # loc/scale or shape/scale for the two-parameter location families). A one-parameter # unbounded family (exponential/chisquare/rayleigh, #417) carries only p1. p2 = float(value[1]) if len(value) >= 2 else None - return FreeParameter(name, keyword, float(value[0]), p2) + # The 3rd token is the native reflecting-bounds flag, which only the Uniform families + # take ('u' -> False, 'b'/absent -> True). It used to be read as inert and dropped, so + # the exporter built a BOUNDED parameter from a conf that declared an unbounded search + # and wrote the box into PEtab's hard bounds without a word (#736). Passed through, so + # the parameter matches the one the fitter builds and petab_parameter_row can refuse + # the shape PEtab has no field for. + bounded = value[2] if len(value) >= 3 else True + return FreeParameter(name, keyword, float(value[0]), p2, bounded=bounded) def _free_parameter_from_conf_record(name, fields): diff --git a/pybnf/petab/parameters.py b/pybnf/petab/parameters.py index de15e2642..72516547a 100644 --- a/pybnf/petab/parameters.py +++ b/pybnf/petab/parameters.py @@ -382,7 +382,9 @@ def petab_parameter_row(free_parameter, parameter_id=None): infinity (half-bounded, ADR-0047). An unbounded one writes blank bounds. The no-prior ``var`` / ``logvar`` point-start keywords and the log forms of the five catalog families (no PEtab ``log-`` spelling) raise ``NotImplementedError`` -- surfaced in - code, not mis-exported. + code, not mis-exported. So does a Uniform family carrying the native ``u`` flag, whose + box seeds the first population without constraining the search: PEtab's bounds are hard, + so there is nothing to write it as (#736, see :func:`_petab_uniform_row`). """ if parameter_id is None: parameter_id = free_parameter.name @@ -406,12 +408,31 @@ def petab_parameter_row(free_parameter, parameter_id=None): def _petab_uniform_row(fp, parameter_id, dist, is_log, nominal): - """A bounded Uniform family -> a PEtab estimated parameter over ``[p1, p2]``. + """A **bounded** Uniform family -> a PEtab estimated parameter over ``[p1, p2]``. The linear ``uniform_var`` needs no ``priorDistribution`` (it *is* PEtab's default for an estimated, prior-less parameter); ``log-uniform`` is not that default, so it states its family and its ``(a, b)`` = the same linear bounds. + + Bounded is the precondition, not a description. The Uniform families are the only + ones carrying the native ``u`` flag (``uniform_var = v 0 10 u``), which turns the + reflecting box *off*: the box seeds the first population and the search is then free + to leave it. That is a statement about where a run starts, and PEtab has no field for + it -- ``lowerBound``/``upperBound`` are hard box constraints -- so writing ``[p1, p2]`` + into them would state a constraint the config explicitly declined (#736). """ + if not fp.bounded: + raise NotImplementedError( + f"Free parameter '{fp.name}' is declared unbounded (the 'u' flag, e.g. " + f"'{fp.type} = {fp.name} {num(fp.p1)} {num(fp.p2)} u'): the box seeds the " + f"first population and the search may then leave it. PEtab v2 cannot say that " + f"-- its lowerBound/upperBound are hard box constraints, and stating the box " + f"as a uniform priorDistribution with blank bounds does not mean it either " + f"(PEtab bounds truncate a prior rather than seeding a draw, and such a row " + f"re-imports as a bounded parameter). Writing [{num(fp.p1)}, {num(fp.p2)}] as " + f"the bounds would export a constraint this job declined, so it is refused " + f"rather than written. Drop the 'u' flag to export the box as real bounds " + f"(ADR-0025, #736).") lb, ub = float(fp.p1), float(fp.p2) prior_distribution = dist if is_log else None prior_parameters = (lb, ub) if is_log else () diff --git a/tests/test_petab_export.py b/tests/test_petab_export.py index 5c12861aa..c6ec9862c 100644 --- a/tests/test_petab_export.py +++ b/tests/test_petab_export.py @@ -181,6 +181,19 @@ def test_no_prior_keyword_export_not_implemented(self): with pytest.raises(NotImplementedError): petab_parameter_row(fp) + @pytest.mark.parametrize('keyword,p1,p2', [ + ('uniform_var', 0.0, 10.0), ('loguniform_var', 0.1, 10.0)]) + def test_unbounded_uniform_export_not_implemented(self, keyword, p1, p2): + # The native 'u' flag turns the reflecting box OFF: the box seeds the first + # population and the search is then free to leave it. PEtab's lowerBound/upperBound + # are hard constraints, so writing [p1, p2] into them would export a constraint the + # job explicitly declined -- refused rather than written (#736). + with pytest.raises(NotImplementedError, match='unbounded'): + petab_parameter_row(FreeParameter('k', keyword, p1, p2, bounded=False)) + # the same declaration with its box ON is untouched + row = petab_parameter_row(FreeParameter('k', keyword, p1, p2, bounded=True)) + assert (row.lower_bound, row.upper_bound) == (p1, p2) + @pytest.mark.parametrize('kind,prefix', [('observable', 'obs_'), ('function', 'func_')]) def test_observable_row_prefix_formula_and_placeholder(self, kind, prefix): # A per-point _SD source -> a declared placeholder bound by noiseParameters. @@ -2133,6 +2146,26 @@ def test_mixed_legacy_and_new_era_is_refused(self, tmp_path): with pytest.raises(NotImplementedError, match='mix'): export_job(conf, tmp_path / 'out') + @pytest.mark.parametrize('line', [ + 'uniform_var = v1 0 10 u', 'loguniform_var = v1 0.1 10 u']) + def test_unbounded_flag_is_refused(self, tmp_path, line): + # End to end: the exporter used to read p1/p2 and drop the flag, so this conf -- + # which declares a search free to leave [0, 10] -- exported to a table byte-identical + # to the bounded spelling, silently constraining the fit a re-import would run + # (#736). Both flag-bearing families raise. + conf = _demo_job(tmp_path, replace=(_V1_VAR, line)) + with pytest.raises(NotImplementedError, match='unbounded'): + export_job(conf, tmp_path / 'out') + + def test_explicit_bounded_flag_exports_as_the_bare_declaration(self, tmp_path): + # The other half of the flag is unchanged: 'b' means what leaving it off means, and + # passing it through the FreeParameter must not perturb the emitted table. + export_job(DEMO_CONF, tmp_path / 'bare') + export_job(_demo_job(tmp_path, replace=(_V1_VAR, _V1_VAR + ' b')), + tmp_path / 'flagged') + assert (tmp_path / 'flagged' / 'parameters.tsv').read_text() == \ + (tmp_path / 'bare' / 'parameters.tsv').read_text() + def test_modern_edition_without_objective_is_refused(self, tmp_path): # New era has no implicit chi_sq default: an edition-2 job must name its objective. with pytest.raises(NotImplementedError):