diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d3b8ee7..065b6820 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 0a8bf998..98d6c1a1 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 143b9e00..1ca90a36 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 e534e5a0..9f7f1015 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 7d60379d..e38ac744 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 de15e264..72516547 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 5c12861a..c6ec9862 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):