`ExperimentsToSbmlConverter`: assign a safe default event priority automatically where possible

[dweindl]

ExperimentsToSbmlConverter documents that "PEtab events are added with a higher priority than the existing events to guarantee that PEtab condition changes are applied before any pre-existing assignments". That guarantee currently holds only for models in which every pre-existing event already carries a numeric priority. In every other case the converter silently produces a model whose event order is undefined, and the consumer has no way to know.

PEtab test suite case 0030 (0030.py) is exactly this situation — a species S in compartment C with dS/dt = p, a model event doubling C at t=10, and a PEtab condition change at that same time:

model petab_test_0030
    compartment C = 4
    species S in C = 3   # overwritten by the condition table
    p = 1
    S' = p
    at time >= 10, fromTrigger=false: C = C * 2
end

with condition1: S = 2, condition2: S = S + C, C = 8, and experiment1 applying condition1 from t=0 and condition2 from t=10. The model event, _E0, carries no priority.

Converting that problem and listing each event's priority before and after:

before: {'_E0': None}
after : {'_E0': None, '_petab_event_experiment1_1': None}

The partial-priority case is no better. Adding one further event that does carry a priority and never collides with anything —

    E1: at time >= 20, priority=5, fromTrigger=false: C = C

before: {'_E0': None, 'E1': '5'}
after : {'_E0': None, 'E1': '5', '_petab_event_experiment1_1': '6 dimensionless'}

Two distinct gaps:

No priorities at all in the model. _max_event_priority stays None, so _petab_event_priority is None and the period-start event gets no Priority subobject either. Nothing orders it against _E0.

Priorities on some events only. _max_event_priority is not None, so the period-start event does get max + 1 — but _E0 keeps no priority, and per SBML §4.12.3 "the set of events without Priority objects must be executed in an undefined order with respect to each other and with respect to the events with Priority subobjects". So the one event that actually collides is still unordered against the condition change, while the priority that was assigned orders the PEtab event only against E1, which never collides with anything. The documented guarantee is quietly not met.

This is not benign. Period-start events are created with useValuesFromTriggerTime=True, so when one executes after a co-timed model event its assignments are evaluated against the pre-cascade state and overwrite that event's effect entirely. Depending on the consumer's internal event order the simulation is either correct or silently wrong, with nothing in the conversion output distinguishing the two. No warning is emitted in either case above — see #525.

Why not simply always assign a default

Because it strengthens the semantics the model demands, rather than merely disambiguating it. Per SBML §4.12.3, events with no Priority may be executed in any order the tool likes, and §8.2.5 explicitly permits a simulator to resolve this by "assign[ing] an arbitrary priority value to all events with undefined priorities … This part of SBML event behavior is left up to developers". But events with mathematically equal declared priorities "must be triggered in a random order", and the spec is emphatic that "a random order is not the same as an undefined order … a randomly-determined order must lead to an equal chance of executing 'A' first or 'B' first, every time those two events are executed simultaneously".

So writing priority 0 onto every previously unprioritised event converts "a tool may pick deterministically" into "a conforming tool must randomise", which can turn currently reproducible simulations into non-reproducible ones. That is presumably why default_priority was left opt-in in #387. The catch is that it pushes the decision onto the consumer, who is usually no better placed to make it.

The asymmetry that makes this tractable

Benefit and risk do not sit on the same pairs of events:

• The priority is needed when a PEtab condition event can execute simultaneously with a model event.
• The priority is risky when two pre-existing model events can execute simultaneously with each other, because they would receive the same default and thereby acquire the mandatory-randomness requirement.

So the only question that has to be answered before assigning a default is: can any two pre-existing model events execute at the same time? If provably not, a default priority is free — it introduces no tie, and it makes the PEtab ordering explicit.

Throughout, "simultaneous" means identical execution times, and everything below that reasons from trigger expressions assumes events without a Delay subobject. That assumption has to be checked in code (Event.isSetDelay()), not taken for granted — see the caveats at the end.

Suggested incremental steps

1. At most one pre-existing event. A tie among model events is then impossible by construction, so assign the default automatically. This is cheap (len(model.getListOfEvents()) <= 1), needs no trigger analysis and no delay check, and covers case 0030.
2. All pre-existing events already prioritised. Handled today; worth an explicit test so the behaviour is pinned rather than incidental.
3. Partial priorities. Assign the default to the unprioritised subset only, subject to the same tie test as step 4 — this is the second case above, and closing it is what makes the docstring's guarantee true.
4. Several events, provably non-simultaneous. Decide from the trigger expressions whether any two model events can share an execution time. The verdict is three-valued and only one branch licenses a default: provably-distinct (safe), provably-coincident (needs a warning or an explicit decision), and undecidable. The undecidable class is large — parameter-dependent trigger times (10 versus p coincide iff p == 10, a runtime fact), triggers gated on conditions, and any state-dependent trigger. Any event with a Delay also belongs there unless the delay is a constant that can simply be added to the predicted trigger time; the check is Event.isSetDelay() and it has to be performed rather than assumed, since an event's execution time is otherwise not readable from its trigger expression at all. Worth starting with the narrow sub-case that covers most real models: triggers of the form time >= c with distinct constant c and no delay.
5. Surface the residue. Where it cannot be decided, warn (which requires fixing ExperimentsToSbmlConverter: "no priority set" warning is unreachable #525 first) and document default_priority as the escape hatch.

Steps 1–3 are mechanical and would cover the cases that currently fail; step 4 is where the real work is.

Two caveats on the analysis

Compare execution times, not trigger times — and confirm there are no delays. §4.12.3 defines simultaneity by execution time, which "is calculated as the sum of the time at which a given event's Trigger is triggered plus its Delay duration, if any". Two events with distinct trigger times can therefore still execute simultaneously — trigger at 10 with delay 5, trigger at 12 with delay 3, both execute at 15. So the delay-free assumption that the rest of this reasoning rests on is not something to state in a docstring and move on from: the implementation must test it (Event.isSetDelay() on every event) and refuse to conclude "provably distinct" when it does not hold. Steps 3 and 4 should either add a constant delay into the comparison or treat any delayed event as undecidable.

The limit is the analysis, not the criterion. Distinct execution times do imply non-simultaneity, cascades included: an event brought into a cascade by another event's assignment triggers at that same time, so it coincides by construction rather than slipping past the test. What cannot always be done is determining those times from the trigger expressions, since a state-dependent trigger may become true only as a consequence of another event's assignment and nothing in the expression reveals it. Such events belong in the undecidable branch of step 4 and must not be counted as provably distinct.

This makes the sub-case suggested for step 4 stronger than it may look: if every model event's trigger depends only on time and no event carries a Delay — both verified, not assumed — then no event assignment can change when any of them fires, so the predicted times are exact and pairwise distinctness is both sound and complete.

References #387, #525

🤖 Generated with Claude Code