`ExperimentsToSbmlConverter`: "no priority set" warning is unreachable

[dweindl]

ExperimentsToSbmlConverter warns when a pre-existing model event has no priority, because in that case the relative order of that event and a co-triggering PEtab condition change is undefined. The guard for that warning can never be true for an event that actually lacks a priority (petab/v2/converters.py:166 on main):

for event in model.getListOfEvents():
    # Check for undefined event priorities and warn
    if (prio := event.getPriority()) and prio.getMath() is None:
        warnings.warn(
            f"Event `{event.getId()}` has no priority set. "
            ...

For an event with no <priority> element at all — the common case, and the one the warning is about — getPriority() returns None, so the walrus target is falsy and the branch is skipped. For an event that does have a priority, prio.getMath() is not None, so the branch is skipped as well. The only input that reaches the warning is a <priority> element that exists but carries no math, which essentially does not occur in practice.

The net effect is that converting a problem whose model has unprioritised events succeeds silently, and the downstream tool is left with undefined event ordering and no indication that anything is ambiguous.

Reproducible with PEtab test suite case 0030, whose model contains one event (_E0) with no priority, co-triggering with a PEtab condition change at t=10:

import warnings
import petabtests
from petab.v2 import Problem
from petab.v2.converters import ExperimentsToSbmlConverter

case_dir = petabtests.get_case_dir("0030", "sbml", "v2.0.0")
problem = Problem.from_yaml(case_dir / petabtests.problem_yaml_name("0030"))

with warnings.catch_warnings(record=True) as caught:
    warnings.simplefilter("always")
    ExperimentsToSbmlConverter(problem).convert()

print([str(w.message) for w in caught])  # no priority warning

The original event reports getPriority() is None, and both events in the converted model end up without a priority, so nothing downstream can distinguish "deliberately undefined" from "nobody thought about it".

The fix is presumably:

if (prio := event.getPriority()) is None or prio.getMath() is None:

This is not just cosmetic for consumers. A simulator with no notion of simultaneous event execution has to fall back on some internal ordering of the two co-timed events, and since the period-start event is created with useValuesFromTriggerTime=True, running it second makes its assignments overwrite the model event's effect entirely. Case 0030 then silently yields wrong results, with nothing in the conversion output hinting at why. The warning, had it fired, would have pointed straight at default_priority.

While here, it may be worth reconsidering whether the warning should be emitted only when an unprioritised event can plausibly co-trigger with a condition change, since for models with many events it will otherwise be noisy.

🤖 Generated with Claude Code