Skip to content

ExperimentsToSbmlConverter: write one flat initial assignment per target - #546

Merged
FFroehlich merged 2 commits into
mainfrom
claude/issue-540-fix-3c75eb
Oct 19, 2026
Merged

FFroehlich merged 2 commits into
mainfrom
claude/issue-540-fix-3c75eb

Conversation

@FFroehlich

@FFroehlich FFroehlich commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

ExperimentsToSbmlConverter built the initial assignments for the first period once per experiment. Each experiment read the current initial assignment back as the default and wrapped it in a new Piecewise, so the expression nested one level deeper per experiment. With a few hundred experiments, reading it back (sbml_math_to_sympy) failed in lxml with Excessive depth in document: 256. This breaks Raimundez_PCB2020 upgraded to v2.

Change

convert() now collects the first-period changes of all experiments and writes them in one go (new _create_initial_assignments): one flat Piecewise per target, with one (value, indicator_e > 0.5) pair per experiment that changes the target, and the original model's initial assignment or initial value as the otherwise branch.

This follows the second fix suggested in the issue rather than flattening while wrapping. It also means the default is always the original model's value, and the growing assignment is no longer serialized and re-parsed once per experiment.

Verification

  • New test test_initial_assignments_many_experiments (300 experiments; a species with an initial value and one with an initial assignment, plus one experiment that changes neither). It checks that the assignment is a flat Piecewise with one pair per experiment and that it evaluates correctly for the first and last experiment and for the default. It fails on main with the lxml error from the issue.
  • The issue's reproducer: the initial assignment of A is 3 levels deep for 5, 50, 300 and 1000 conditions (before: 7, 52, lxml error).
  • Benchmark collection (all models except Froehlich_CellSystems2018, upgraded with petab1to2): for every initial assignment and every experiment (and with all indicators at 0), the old and new converter give the same value. The maximum MathML depth drops, e.g., Alkan_SciSignal2018 75 → 4, Bachmann_MSB2011 38 → 4, Smith_BMCSystBiol2013 37 → 4. Raimundez_PCB2020 (170 experiments) fails with the old converter. With the new one, all 1360 first-period changes match the resulting initial assignments, at depth 4. Conversion takes ~2 s; the old converter took 22 s before failing.
  • pytest tests/v2: 142 passed, 1 skipped (with benchmark_models_petab installed). pre-commit is clean.

Closes #540

🤖 Generated with Claude Code

The first-period changes were turned into initial assignments once per
experiment. Each experiment read the current initial assignment back as
the default and wrapped it in a new Piecewise, so the expression nested
one level deeper per experiment. With a few hundred experiments, reading
it back failed in lxml ("Excessive depth in document: 256"), e.g., for
Raimundez_PCB2020 upgraded to v2.

Now the first-period changes of all experiments are collected and
combined into a single flat Piecewise per target after all experiments
were converted. The default is the initial assignment or initial value
of the original model.

Closes #540

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.07%. Comparing base (5fb4066) to head (b252841).

Files with missing lines Patch % Lines
petab/v2/converters.py 86.36% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
+ Coverage   77.98%   78.07%   +0.08%     
==========================================
  Files          68       68              
  Lines        7791     7793       +2     
  Branches     1390     1389       -1     
==========================================
+ Hits         6076     6084       +8     
+ Misses       1227     1224       -3     
+ Partials      488      485       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@FFroehlich
FFroehlich marked this pull request as ready for review October 15, 2026 08:41
@FFroehlich
FFroehlich requested a review from a team as a code owner October 15, 2026 08:41

@dweindl dweindl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@FFroehlich
FFroehlich disabled auto-merge October 19, 2026 08:15
@FFroehlich
FFroehlich merged commit 8bbe776 into main Oct 19, 2026
13 checks passed
@FFroehlich
FFroehlich deleted the claude/issue-540-fix-3c75eb branch October 19, 2026 08:16
FFroehlich added a commit that referenced this pull request Oct 19, 2026
… once (#556)

Extends `assignments_to_experiments` (#527) to assignments that switch
more than once.

**Why.** Of the benchmark-collection problems with time-dependent
piecewise assignments, `main` converts two (Isensee_JCB2018,
Raimundez_PCB2020) and rejects three. Two of those have an exact form as
experiment periods:

- **Brannmark_JBC2010**, a sum of doses given at different times:
`insulin := insulin_dose_1 * piecewise(0, time - insulin_time_1 < 0, 1)
+ insulin_dose_2 * piecewise(0, time - insulin_time_2 < 0, 1)`
- **Oliveira_NatCommun2021**, phases one after another as nested
piecewise functions:
`beta := piecewise(beta_0, time < t_1, piecewise(beta_1, time < t_2,
...))`

**What.** An assignment may now depend on time through any number of
piecewise conditions — in a sum, in several pieces, or nested — within
the scope agreed in #527, applied per condition:

- every condition is a single inequality that can be re-arranged into `t
</<=/>/>= ...` (no `&&`/`||`, no equality, linear in time with a
non-zero slope);
- every time-dependent piecewise function has an explicit "otherwise"
piece;
- time enters only through those conditions;
- every switching time evaluates to a real number in each experiment: it
may depend on constant model parameters, on parameters fixed in the
parameter table, or on parameters the condition table sets, but not on
estimated parameters;
- all other symbols are truly constant model parameters.

The value on each interval between switches is the assignment with every
condition replaced by whether it holds there. A switch that changes the
value becomes a period; one that does not (e.g. two phases with equal
values) adds nothing.

The value before every switch follows from each condition's slope and is
kept in the model. If it refers to parameter table symbols (e.g. an
estimated `d1` in `piecewise(d1, time < 10, d2)`), it is also set at the
start of the first period, since the model can only hold a number for
it. (On `main`, such an assignment fails with a `TypeError`.)

Switching times are checked up front, in `_supported_time_switches`: one
that depends on an estimated parameter is rejected with an error naming
those parameters. Otherwise, the symbols of a switching time that no
condition sets take their value from the parameter table, if fixed
there, or else from the model. On `main`, a switching time given by a
constant model parameter is rejected. A condition that sets a switching
time to an expression in estimated parameters is still rejected per
experiment.

Everything that converts on `main` converts as before: all existing
tests pass unchanged except in
`test_assignments_to_experiments_unsupported`, where the two cases
asserting that these two shapes are rejected are replaced by a
conjunction (`time < 10 && time > 5`), still rejected, and the estimated
switching time now expects the new error. The docstring's scope is
updated accordingly; the preequilibration warning and the final lint are
unchanged.

**Checked**

- New tests: a sum of doses, several pieces, nested pieces, a switch
that does not change the value, and condition-specific doses as in
Brannmark (each experiment gets its own switching times and amounts);
switching times given by a constant model parameter, a parameter fixed
in the parameter table, or the condition table; a condition that sets a
switching time to an estimated parameter (rejected); a pre-switch value
that refers to estimated parameters.
- On the benchmark problems (`assignments_to_experiments` after
`petab.v2.Problem.from_yaml`, then simulated with AMICI's PEtab v2
importer at the nominal parameters, atol 1e-10):

  | problem | `main` | this branch |
  |---|---|---|
| Brannmark_JBC2010 | rejected | converted (17 periods); llh −141.889201
→ −141.889187 |
| Oliveira_NatCommun2021 | rejected (shape) | rejected: its switching
times `t_1`, `t_2` are **estimated**, which no period time can be; the
error names them |
| Fujita_SciSignal2010 | rejected | rejected: `EGF_rate*time` puts time
in a value |
  | Isensee_JCB2018 | converted, Δllh −1.8e-3 | unchanged |
| Raimundez_PCB2020 | converted, Δllh 4.7e-5 | unchanged (simulated with
#546) |

So Oliveira does not become convertible — it now fails for the reason
that actually prevents it, rather than for its shape.

The fixed switching times and the pre-switch value were added after the
simulations above. They leave the converted Brannmark, Isensee and
Raimundez problems identical (experiment table, condition table, SBML),
so those log-likelihoods still hold.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01MNJePorYs9zmPYMam6g4EP

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ExperimentsToSbmlConverter nests initial-assignment Piecewise once per experiment, exceeding lxml's depth limit

3 participants