assignments_to_experiments: slim down by FFroehlich · Pull Request #573 · PEtab-dev/libpetab-python

FFroehlich · 2026-10-25T14:19:11Z

Follows #559. A refactor of assignments_to_experiments that does not change its behaviour. The fixes for bugs found while reviewing and stress-testing it are in #574, stacked on this PR.

Slimming down `assignments_to_experiments`

The function had grown to 340 lines, most of them one loop body passing around the parsed assignment, its switches, its value before every switch, the switching times' fixed values and the slope parameter of a ramp. It is now about 75 lines (plus its docstring) that run the steps in order:

_add_start_periods: give every experiment the period that starts it, and find the periods that may be dropped;
for each time-dependent assignment rule, a _PiecewiseAssignment:
- its constructor checks that the assignment is supported and works out the value before every switch, the switching times' fixed values and whether the value ramps;
- replace_in_model() replaces the rule in the model (the slope parameter and rate rule for a ramp, the target's value);
- changes(experiment) returns the condition changes that replace it in an experiment. The old module-level _changes and _value_from became its methods.
_drop_changes / _add_conditions: replace the condition changes the new conditions cover;
drop empty periods, add the default experiment, lint.

The redundant / after_preequilibration double negative became one decision per period: one branch for the first period, one for the later ones.

On the review comment on #557 about not turning this into a class: assignments_to_experiments stays a function. _PiecewiseAssignment only holds the values of one assignment that the old loop body passed between its parts. If you would rather not have it, it can become a few free functions that take those values.

No change in behaviour: on all 40 conversions in the tests, the old and new implementations give the same experiment table, condition table, measurement table, SBML, errors and warnings. The benchmark problems Brannmark_JBC2010, Fujita_SciSignal2010, Isensee_JCB2018, Raimundez_PCB2020 (and Oliveira_NatCommun2021's error) also convert identically.

Checked

pytest tests/v2 tests/v1/test_petab.py: 244 passed; pre-commit clean.

🤖 Generated with Claude Code

… rate rule A condition change is applied once at the start of its period, with `time` set to the period's start, so a value that changes with time within a period cannot be a condition change on a constant target, and such assignments were rejected. Fujita_SciSignal2010's EGF input is one: `piecewise(EGF_rate * time + EGF_0, time <= EGF_end, 0)`, steps in training and ramps in its held-out data. A value may now change linearly with time. The target then becomes a differential entity, with the rate rule `d target/dt = <target>_slope` and a new constant parameter for the slope, and each period sets both the target's value at the period start and its slope. During preequilibration the value is held at its value at time 0 with slope zero, and the first period after preequilibration sets the slope again. An assignment that only switches between constant values converts exactly as before, as a constant parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNJePorYs9zmPYMam6g4EP

…ifferential The slope parameter is a placeholder at 0; every ramp's start value and slope, including at time 0, are set through the condition table. Constant model parameters in changes at an experiment's start take their model values, since those changes may only refer to parameter table symbols. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNJePorYs9zmPYMam6g4EP

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

`add_sbml_parameter` checks libsbml's return codes, which the hand-written parameter creation did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A period's time overrides the model's initial time, so an experiment starts at its first period after preequilibration. The converter dropped that period when nothing changed there, either as redundant, since the model already holds the value before the switch, or because its condition only set symbols that the conversion replaced. The experiment then started at its first switch, and measurements before the switch were simulated from the wrong start. The period that starts each experiment now stays, without conditions if none are left. A switch therefore never becomes an experiment's first period, and a start value that equals the model's no longer needs to be set when a switch refers to symbols outside the parameter table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… period Since the experiment keeps the period that starts it, the switch's condition is at the second period, after an empty one at t = 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… rate rule A condition change is applied once at the start of its period, with `time` set to the period's start, so a value that changes with time within a period cannot be a condition change on a constant target, and such assignments were rejected. Fujita_SciSignal2010's EGF input is one: `piecewise(EGF_rate * time + EGF_0, time <= EGF_end, 0)`, steps in training and ramps in its held-out data. A value may now change linearly with time. The target then becomes a differential entity, with the rate rule `d target/dt = <target>_slope` and a new constant parameter for the slope, and each period sets both the target's value at the period start and its slope. During preequilibration the value is held at its value at time 0 with slope zero, and the first period after preequilibration sets the slope again. An assignment that only switches between constant values converts exactly as before, as a constant parameter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNJePorYs9zmPYMam6g4EP

…ifferential The slope parameter is a placeholder at 0; every ramp's start value and slope, including at time 0, are set through the condition table. Constant model parameters in changes at an experiment's start take their model values, since those changes may only refer to parameter table symbols. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MNJePorYs9zmPYMam6g4EP

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

`add_sbml_parameter` checks libsbml's return codes, which the hand-written parameter creation did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#557 was rebased onto main (with the pandas fix #571), so #559 conflicted with its base. The result is the same tree as #559 rebased onto #557. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

#557 was squash-merged into main, so #559 conflicted with main. The result is the same tree as before, since main now has exactly #557's content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The function had grown to 340 lines, most of them one loop body passing around the parsed assignment, its switches, its value before every switch, the switching times' fixed values and the slope parameter of a ramp. These now live on `_PiecewiseAssignment`: its constructor checks that the assignment is supported, `replace_in_model` replaces it in the model, and `changes` returns the condition changes that replace it in an experiment. `_changes` and `_value_from` become its methods. The "redundant" check becomes a decision per period, one branch for the first period and one for the later ones. Setting up the start periods, dropping the replaced changes and adding the new conditions are separate functions. No change in behaviour: the old and new implementations give the same tables, SBML, errors and warnings on all conversions in the tests, and on the benchmark problems Brannmark_JBC2010, Fujita_SciSignal2010, Isensee_JCB2018, Oliveira_NatCommun2021 and Raimundez_PCB2020. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ope gaps - A switch exactly at the start of a later period was lost when that period did not change any symbol of the assignment, e.g. `piecewise(0, time < 10, 1)` with a period at t = 10, or `time <= 0` after preequilibration. A later period now also sets the value when it differs from the value at the end of the previous period. - The condition changes of every symbol of a converted assignment were dropped, even when the model still used the symbol elsewhere, e.g. a dose that also enters a reaction. They are now only dropped if nothing else (the model, an observable, a condition change) refers to the symbol. - Time-dependent initial assignments were converted into switches, but SBML evaluates them once, at the start of the simulation. They now stay in the model. - At preequilibration, which starts the experiment, a value that referred to model parameters outside the parameter table failed the lint. Those parameters now take their model values, as at the first period. - An assignment to a species or compartment crashed with an AttributeError; it now raises NotImplementedError. - A problem without time-dependent assignments was still changed: an experiment that only preequilibrates got an empty period at t = 0. `test_assignments_to_experiments` and `_unused_condition_id` used an initial assignment to mean a switching rule and now use an assignment rule. `_validate` relied on an estimated parameter with a time-dependent initial assignment; it now uses a ramp whose generated slope parameter collides with an observable parameter. The converted benchmark problems (Brannmark_JBC2010, Fujita_SciSignal2010, Isensee_JCB2018, Raimundez_PCB2020) are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

codecov-commenter · 2026-10-25T14:21:43Z

Codecov Report

❌ Patch coverage is 97.45763% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.97%. Comparing base (2b08040) to head (d9ac296).

Files with missing lines	Patch %	Lines
petab/v2/converters.py	97.45%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
+ Coverage   78.91%   78.97%   +0.06%     
==========================================
  Files          69       69              
  Lines        8072     8096      +24     
  Branches     1449     1450       +1     
==========================================
+ Hits         6370     6394      +24     
  Misses       1215     1215              
  Partials      487      487

☔ 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.

…-experiments-refactor # Conflicts: # petab/v2/converters.py

…nconverted problems alone, pick an unused slope ID - A parameter used as a model or species conversion factor counted as unused, so a condition change of it was dropped when the parameter also appeared in a converted assignment. Conversion factors now count as references. - A problem without time-dependent assignments was still changed: its experiments' periods were re-sorted. It is now returned as it was, after the lint. - A ramp's slope parameter was only named so as not to clash with SBML IDs, and clashed with, e.g., an observable parameter `<target>_slope`. It is now named with `Problem.get_unused_id`. `test_assignments_to_experiments_validate` relied on that slope clash to fail the lint; it now uses an existing experiment `default_experiment`, which the measurements without an experiment ID are assigned to as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…start of its period The converter kept a condition change's value as an expression and substituted it wherever a switch used it, so the value was re-evaluated at the switch with the values at that time. A condition change is evaluated once, at the start of its period, and all changes of a period simultaneously, with the values from before the period. E.g., with `d1 = b1 + 0.5` at t = 2 and `b1 = 2` at t = 3, a switch to `d1` at t = 5 used 2.5 instead of 1.7; likewise with `b1 = 2` in the same condition as `d1`. A change's value is now evaluated at the start of its period: symbols set by an earlier condition take that value, other constant model parameters their model values, and parameter table symbols stay. A change of a symbol of the assignment whose value still refers to anything else, e.g. a species, is rejected, since its value is not known there. Found by differential testing against simulations of the original and the converted problem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This PR keeps only the refactor of `assignments_to_experiments`, which does not change its behaviour. The fixes, which do, follow in a separate PR stacked on this one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…te evaluation of condition changes (#574) Follows #573. Fixes in `assignments_to_experiments`, found while reviewing #573's refactor and by differential testing against simulation. Each one changes the converted problem, and each has a regression test that fails without the fix. None was introduced by the refactor. ## Fixes - **A switch exactly at the start of a later period was lost** when that period did not change any symbol of the assignment. With `k1 := piecewise(0, time < 10, 1)` and periods at 0 and 10, k1 stayed 0 after t = 10. Likewise `time <= 0` after preequilibration. A later period now also sets the value when it differs from the value at the end of the previous period. - **Condition changes of symbols still used elsewhere were dropped.** With `k1 := dose * piecewise(0, time < 10, 1)` and a reaction that also uses `dose`, the condition's `dose = 2` was removed, so the reaction silently used the model value. Changes are now only dropped if nothing else (the model's math or conversion factors, an observable, a condition change) refers to the symbol. - **A condition change was re-evaluated wherever a switch used it**, with the values at the switch. PEtab applies a condition once, at the start of its period, and all its changes simultaneously, with the values from before the period. With `d1 = b1 + 0.5` at t = 2 and `b1 = 2` at t = 3 (or `b1 = 2` in the same condition), a switch to `d1` at t = 5 used 2.5 instead of 1.7. Changes are now evaluated at the start of their period; one whose value refers to something not known there, e.g. a species, is rejected. As a side effect, switching times set to expressions such as `t1 = t2 + 1` now convert instead of being rejected. - **Time-dependent initial assignments were converted into switches**, but SBML evaluates an initial assignment once, at the start of the simulation, so `k1 = piecewise(0, time < 10, 1)` never switches. They now stay in the model, as the docstring's scope ("assignment rules") already said. - **At preequilibration, a value referring to model parameters outside the parameter table failed the lint** (`CheckInitialChangeSymbols`), since preequilibration starts the experiment. Those parameters now take their model values, as at the first period. - **An assignment to a species or compartment crashed** with an `AttributeError`; it now raises `NotImplementedError`. - **A problem without time-dependent assignments was still changed**: an experiment that only preequilibrates got an empty period at t = 0, and periods were re-sorted. It is now returned as it was, after the lint. - **A ramp's slope parameter could clash with PEtab IDs**, e.g. an observable parameter `k1_slope`, since it was only checked against SBML IDs. It is now named with `Problem.get_unused_id`. - **A switching time that a condition set from a parameter fixed by the parameter table was rejected**, e.g. `t1 = p + 1`, although `t1 := p + 1` in the assignment itself converts with `p`'s nominal value. Fixed parameters now take their nominal values in condition changes too; switching times that depend on estimated parameters are still rejected. - **New condition values used the symbols parsed from SBML**, without the assumptions of PEtab's symbols (`petab_symbol`, real), so they compared unequal to the same expressions parsed by PEtab, e.g. after reading the condition table back. They now use PEtab's symbols. Test changes: `test_assignments_to_experiments` and `_unused_condition_id` used an initial assignment (`k1 = ...`) to mean a switching rule and now use an assignment rule (`k1 := ...`). `_validate` relied on an estimated parameter with a time-dependent initial assignment; it now uses an existing experiment `default_experiment`, to which the measurements without an experiment ID would also be assigned, which the final lint catches. ## Module As suggested in review, `assignments_to_experiments` and its helpers now live in their own module, `petab.v2.assignments_to_experiments` (moved unchanged; still exported from `petab.v2.converters`), and their tests in `tests/v2/test_assignments_to_experiments.py`. `SbmlModel.get_referenced_ids()` and `ObservableTable.free_symbols` are new, also from review. ## Checked - `pytest tests/v2 tests/v1/test_petab.py tests/v1/test_sbml.py`: 266 passed; pre-commit clean. - The benchmark problems Brannmark_JBC2010, Fujita_SciSignal2010, Isensee_JCB2018 and Raimundez_PCB2020 (and Oliveira_NatCommun2021's error) convert identically before and after. - Differential testing against simulation: random problems with dose sums, several and nested pieces, ramps, every inequality form, switching times set by conditions (often exactly at period starts), symbols shared with reactions, two converted assignments in one model, and condition values that are expressions in other parameters. Each experiment of the original and the converted problem is turned into SBML events with `ExperimentsToSbmlConverter`, simulated in BasiCO, and compared (rtol 1e-6). On this PR's head, 2093 problems match and none is rejected; in 7 more, COPASI fails on the original model, and the converted one matches it up to there. The same generator finds 59 mismatches in 300 problems on `main`. Not covered: preequilibration, experiments that start at t ≠ 0, and first periods with expression values (only parameter table symbols are allowed there). - A second pass with an independent reference simulator (SciPy, integrating piecewise between all switching times and period starts; it agrees with BasiCO on 94 cross-checked simulations), covering what the first could not: experiments that start at t = -5, 2 or 5, preequilibration (with the documented semantics: converted assignments hold their value at time 0), parameter table entries whose estimated or fixed nominal values differ from the model's (in values, switching times and condition values), condition values that refer to parameter table symbols at the start and to a species in unrelated changes. Each problem also checks that the input stays unchanged, that conversion is deterministic and idempotent, and that the converted condition and experiment tables survive a round trip through their DataFrames. On this PR's head, 1415 of 1500 problems match and pass all checks; the other 85 are rejected because a switching time depends on an estimated parameter, as intended. On `main`, the same pass finds 21 mismatches and 16 lint failures of the converted problem in 200 problems. - Where BasiCO disagrees with the reference simulator on an original problem (a switch of the assignment rule exactly at the time of a period's event, which COPASI mis-simulates; moving the switch by 0.001 removes the disagreement), the reference simulator agrees with the converted problem. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

FFroehlich and others added 14 commits October 25, 2026 12:52

assignments_to_experiments: rename the period index to i_period

304dfe7

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

assignments_to_experiments: add a ramp's slope with add_sbml_parameter

54328e6

`add_sbml_parameter` checks libsbml's return codes, which the hand-written parameter creation did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test_assignments_to_experiments_unused_condition_id: expect the start…

7bc856c

… period Since the experiment keeps the period that starts it, the switch's condition is at the second period, after an empty one at t = 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

assignments_to_experiments: rename the period index to i_period

1c90569

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

assignments_to_experiments: add a ramp's slope with add_sbml_parameter

c52a56f

`add_sbml_parameter` checks libsbml's return codes, which the hand-written parameter creation did not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge #557's rebased branch into #559

75f57a8

#557 was rebased onto main (with the pandas fix #571), so #559 conflicted with its base. The result is the same tree as #559 rebased onto #557. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge main into #559

0606381

#557 was squash-merged into main, so #559 conflicted with main. The result is the same tree as before, since main now has exactly #557's content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Base automatically changed from claude/assignments-to-experiments-start-time to main October 25, 2026 15:48

FFroehlich and others added 4 commits October 25, 2026 15:50

Merge remote-tracking branch 'origin/main' into claude/assignments-to…

f15d6e6

…-experiments-refactor # Conflicts: # petab/v2/converters.py

Move the fixes to their own PR

d9ac296

This PR keeps only the refactor of `assignments_to_experiments`, which does not change its behaviour. The fixes, which do, follow in a separate PR stacked on this one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

FFroehlich mentioned this pull request Oct 25, 2026

assignments_to_experiments: fix lost switches, dropped changes and late evaluation of condition changes #574

Merged

FFroehlich changed the title ~~assignments_to_experiments: slim down, and fix lost switches and dropped changes~~ assignments_to_experiments: slim down Oct 25, 2026

FFroehlich added this pull request to stack #575 October 25, 2026 16:51

FFroehlich marked this pull request as ready for review October 25, 2026 16:51

FFroehlich requested a review from a team as a code owner October 25, 2026 16:51

dweindl approved these changes Oct 25, 2026

View reviewed changes

FFroehlich merged commit 597c0a6 into main Oct 25, 2026
13 checks passed

FFroehlich deleted the claude/assignments-to-experiments-refactor branch October 25, 2026 17:15

Navigation Menu

Repository navigation

assignments_to_experiments: slim down - #573

assignments_to_experiments: slim down#573
FFroehlich merged 18 commits into
mainfrom
claude/assignments-to-experiments-refactor

FFroehlich commented Oct 25, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 25, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

FFroehlich commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Slimming down assignments_to_experiments

Checked

Uh oh!

codecov-commenter commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FFroehlich commented Oct 25, 2026 •

edited

Loading

Slimming down `assignments_to_experiments`

codecov-commenter commented Oct 25, 2026 •

edited

Loading