Skip to content

Make flattened PEtab v2 observable IDs independent of libpetab serialization - #3398

Closed
FFroehlich wants to merge 2 commits into
mainfrom
fix-flatten-override-ids
Closed

FFroehlich wants to merge 2 commits into
mainfrom
fix-flatten-override-ids

Conversation

@FFroehlich

Copy link
Copy Markdown
Member

main has been red on PEtab Testsuite since libpetab-python's #545 ("Fix invalid and lossy output of petab_math_str") was merged: python/tests/petab_/test_petab_v2.py::test_flatten_timepoint_specific_output_overrides fails.

Cause: flatten_timepoint_specific_output_overrides and unflatten_simulation_df built the IDs of the replicated observables from libpetab's measurement table serialization (MeasurementTable.to_df() → petab.v1.core.get_observable_replacement_id). Before #545, overrides were written with sympy's str, i.e. truncated to 15 significant digits (1.0 → 1.00000000000000, giving obs1__obsParOverride1_1_00000000000000__noiseParOverride1). The test encoded those IDs. Since #545, numbers are written exactly and integer-valued floats as integers, so the ID is now obs1__obsParOverride1_1__noiseParOverride1.

Fix: The IDs are now built directly from the Measurement objects' override values, so they no longer depend on how libpetab writes tables:

  • Parameter IDs are kept as they are.
  • Numbers use their shortest representation that round-trips (repr(float)), with p for the decimal point, m for minus signs, and integer-valued numbers without a decimal point. For example, 1.0 → 1, 0.1 → 0p1, -1.5e-07 → m1p5em07. Distinct doubles get distinct IDs, and 1 and 1.0 get the same ID.
  • The ID structure is unchanged: {observableId}__{observableParameters joined by _}__{noiseParameters joined by _}.
  • If two different override combinations still map to the same ID, a RuntimeError is raised. This can happen with parameter IDs that contain underscores, e.g. a_b;c vs. a;b_c. Previously such observables were merged silently, which left timepoint-specific overrides in place.
  • The new observables are ordered by first occurrence in the measurement table. Previously the order came from a pandas groupby on the serialized strings.

The old serialization-based IDs had further problems, with either libpetab version:

  • Negative numbers and numbers in exponential notation produced invalid PEtab IDs (-1_5, 1e-20), so flattening failed with a pydantic ValidationError.
  • With libpetab before Update sundials to 4.x.x #545, overrides that differ only beyond the 15th significant digit got the same ID.
  • With libpetab after Update sundials to 4.x.x #545, . → _ makes, e.g., 1.5;2 and 1;5.2 collide.

Why not just update the expected IDs or pin libpetab: The expected IDs in the test do change, from the truncated 1_00000000000000 to 1. Any exact, compact number format gives 1 here, and the new IDs match what libpetab main produces for this case. But updating only the test would keep the IDs tied to libpetab's table writer, along with the problems above. Pinning libpetab would only postpone the failure.

Tests:

  • Updated the expected IDs in test_flatten_timepoint_specific_output_overrides.
  • Added test_flatten_timepoint_specific_output_overrides_numeric. It covers adjacent doubles (0.1 vs. nextafter(0.1, 1)), negative numbers, exponents, 2 vs. 2.0, and an unflatten_simulation_df round-trip.
  • Added test_flatten_timepoint_specific_output_overrides_ambiguous, which checks the collision error.
  • Both new tests fail with the implementation on main, under both libpetab versions.

Verified locally (Python 3.14, macOS), with libpetab main (663b383, includes #545) and with the previous main (539333b):

  • All 25 tests in python/tests/petab_/test_petab_v2.py pass with both versions.
  • PEtab v2 test suite case 0006 (SBML), which goes through flatten and unflatten_simulation_df, passes with both versions.
  • The PySB variant of 0006 was not run locally because BioNetGen isn't installed here.

🤖 Generated with Claude Code

…ization

`flatten_timepoint_specific_output_overrides` and `unflatten_simulation_df`
derived the IDs of the replicated observables from libpetab's measurement
table serialization. Since libpetab-python#545, numbers are written without
truncation, which changed the IDs (`1.0` -> `1` instead of
`1_00000000000000`) and broke `test_flatten_timepoint_specific_output_overrides`.

The IDs are now built directly from the measurements' override values:
parameter IDs as they are, numbers in their shortest representation that
round-trips, with `p` for the decimal point and `m` for minus signs. This
also fixes collisions of numbers that differ only beyond the 15th
significant digit, and invalid IDs for negative numbers and numbers in
exponential notation. An error is raised if different overrides would still
result in the same ID (e.g. parameter IDs with underscores).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@FFroehlich
FFroehlich requested a review from a team as a code owner October 15, 2026 11:26
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@FFroehlich
FFroehlich marked this pull request as draft October 15, 2026 11:28
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.98%. Comparing base (3f3c9b0) to head (de197bd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3398   +/-   ##
=======================================
  Coverage   79.98%   79.98%           
=======================================
  Files         320      320           
  Lines       22674    22707   +33     
  Branches     1531     1531           
=======================================
+ Hits        18135    18162   +27     
- Misses       4531     4537    +6     
  Partials        8        8           
Flag Coverage Δ
cpp 74.04% <9.09%> (-0.05%) ⬇️
cpp_python 35.07% <9.09%> (-0.01%) ⬇️
petab 48.56% <100.00%> (+0.09%) ⬆️
petab_sciml 16.88% <9.09%> (+<0.01%) ⬆️
petab_sciml_benchmarks 15.46% <9.09%> (+<0.01%) ⬆️
python 72.22% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...hon/sdist/amici/importers/petab/_petab_importer.py 88.94% <100.00%> (+0.21%) ⬆️
python/tests/petab_/test_petab_v2.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@FFroehlich

Copy link
Copy Markdown
Member Author

Closing: the failing test was already fixed by #3394.

@FFroehlich FFroehlich closed this Oct 15, 2026
@FFroehlich
FFroehlich deleted the fix-flatten-override-ids branch October 15, 2026 14:57
@dweindl dweindl added the PEtab PEtab-import related label Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PEtab PEtab-import related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants