v2: Add experiments->events converter

[dweindl]

Add functionality to convert PEtab v2 experiments/conditions to SBML events. This should make it easier to implement v2 support in other tools.

All condition table changes are converted to event assignments. Only two indicator variables have to be set for each experiment.

Closes #370.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 73.85621% with 40 lines in your changes missing coverage. Please review.

Project coverage is 74.15%. Comparing base (314c495) to head (9164e19).

Files with missing lines	Patch %	Lines
petab/v2/converters.py	72.86%	23 Missing and 12 partials ⚠️
petab/v2/models/_sbml_utils.py	69.23%	1 Missing and 3 partials ⚠️
petab/v2/problem.py	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   74.01%   74.15%   +0.13%     
==========================================
  Files          60       62       +2     
  Lines        6642     6794     +152     
  Branches     1154     1184      +30     
==========================================
+ Hits         4916     5038     +122     
- Misses       1288     1307      +19     
- Partials      438      449      +11     

☔ View full report in Codecov by Sentry.
📢 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.

[dilpath]

Nice 👍

[dilpath]

Hm, makes sense since SBML distinguishes "simultaneous due to undefined priority" and "random ordering due to equal priority". Still, I guess setting the priority and issuing a warning might make more sense/is more user-friendly than not setting the priority. The latter case supports users who exploit tool-specific ordering of undefined events, which I guess is relatively rare.

[dweindl]

simultaneous due to undefined priority

The order would be undefined in that case, not simultaneous.

exploit tool-specific ordering of undefined events

This is definitely not a use case I would want to support in PEtab.

[dilpath]

simultaneous due to undefined priority

The order would be undefined in that case, not simultaneous.

exploit tool-specific ordering of undefined events

This is definitely not a use case I would want to support in PEtab.

Yes, the events are simultaneous, and the ordering is undefined. But my point was just that setting a priority changes the ordering. Hence, we could set the priority of the event, and change the warning to be "this event priority is now defined". I guess this is more user friendly, because users are probably not trying to exploit (or rather, we don't want to support) tool-specific behavior for "events that are simultaneous, with undefined ordering".

[dweindl]

Hm, I don't think this changes anything. If there are two event assignments to be executed at the same simulation time, it doesn't matter if one has a priority and the other does not, or if neither has a priority. The execution order is undefined in either case.

[dilpath]

Sorry, the complete suggestion:
The user has a model that already has an event A that will trigger at t=10, with undefined priority.
The converter adds a period condition change event B at t=10.

We can set the priority of both event A and event B, to get the expected behavior for the PEtab condition change, and warn the user that the event A priority was changed.

[dilpath]

But there is no choice: the PEtab event must occur first. This ordering must already be accepted by the user regardless of this experiment->event converter, since this ordering is in the PEtab spec. The user's only choice is whether the ordering of their remaining non-PEtab events are undefined or random or ordered -- which is the thing that the user does not care about, or they would have already set the priority. Hence, I see setting a priority as the sensible default, since it won't break anything, and will ensure that the PEtab event occurs first?

[dweindl]

While you are right in principle, I still don't think this is the place to change the behavior of other parts of the model. I am fine with adding an additional default_priority parameter to set any undefined priorities to that value if a user opts in.

[dilpath]

I still don't think this is the place to change the behavior of other parts of the model

Could happen but if behavior does change, it would be behavior that is not within the scope of SBML, as far as I can tell, since SBML leaves it undefined.

Anyway, a default_priority (or preexisting_priority so it's clear that it's different to the PEtab event priority) would be fine for me. Should be included in this warning message, so users know how to quickly resolve the warning.

[dweindl]

I'd find preexisting_priority somewhat confusing, since it's specifically used for events for which no priority preexists.

[dilpath]

Hm, maybe undefined_priority?

Also the new docstring for default_priority describes the "undefined priority" case, but not the "equal priority" case, which is what using the default_priority will result in.

On both points, the current PR state is fine for me 👍 Thanks