Repository navigation
ExperimentsToSbmlConverter: write one flat initial assignment per target - #546
Merged
Merged
Conversation
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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
FFroehlich
marked this pull request as ready for review
October 15, 2026 08:41
FFroehlich
enabled auto-merge (squash)
October 15, 2026 08:51
This was referenced Oct 16, 2026
FFroehlich
disabled auto-merge
October 19, 2026 08:15
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ExperimentsToSbmlConverterbuilt 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 newPiecewise, so the expression nested one level deeper per experiment. With a few hundred experiments, reading it back (sbml_math_to_sympy) failed in lxml withExcessive depth in document: 256. This breaksRaimundez_PCB2020upgraded to v2.Change
convert()now collects the first-period changes of all experiments and writes them in one go (new_create_initial_assignments): one flatPiecewiseper 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 theotherwisebranch.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
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 flatPiecewisewith one pair per experiment and that it evaluates correctly for the first and last experiment and for the default. It fails onmainwith the lxml error from the issue.Ais 3 levels deep for 5, 50, 300 and 1000 conditions (before: 7, 52, lxml error).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 (withbenchmark_models_petabinstalled).pre-commitis clean.Closes #540
🤖 Generated with Claude Code