Update `ExperimentsToEventsConverter` to changed initialization semantics #443

dweindl · 2025-09-30T19:38:44Z

... the first period of a PEtab experiment has to be applied before initial assignments are evaluated. That means, the changes have to be implemented as initial assignments instead of event assignments at the initial timepoint (or any pre-existing initial assignments would have to be included in the event assignment).
... for any subsequent periods, the event assignments need to be modified. Compartment-size changes in PEtab no longer follow the SBML event assignment semantics. That means, we need event assignments for all concentration-based species inside such a compartment to preserve concentrations instead of amounts.

Closes #452.

codecov-commenter · 2025-09-30T19:46:06Z

Codecov Report

❌ Patch coverage is 71.42857% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.79%. Comparing base (86e0290) to head (778935a).

Files with missing lines	Patch %	Lines
petab/v2/converters.py	71.42%	10 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   74.86%   74.79%   -0.07%     
==========================================
  Files          62       62              
  Lines        6790     6845      +55     
  Branches     1204     1223      +19     
==========================================
+ Hits         5083     5120      +37     
- Misses       1251     1258       +7     
- Partials      456      467      +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

Looks good 👍

petab/v2/converters.py

dilpath · 2025-10-02T13:36:43Z

petab/v2/converters.py

+                # combine all changes into a single piecewise expression
+                if expr_cond_pairs := [
+                    (target_value, sp.Symbol(exp_ind) > 0.5)
+                    for exp_ind, target_value in changes
+                    if target_value != default
+                ]:
+                    # only create the initial assignment if there is
+                    #  actually something to change
+                    expr = sp.Piecewise(
+                        *expr_cond_pairs,
+                        (default, True),
+                    )


I think the comments above and below the if are swapped.

Also is this check only relevant when the condition table happens to contain the same value as the SBML default? Or is there another case where this becomes relevant? IMO fine to skip this check if it's just about cases where the user coincidentally duplicates the SBML default into the condition table -- if they have redundant information in the table then redundant initial assignments can be the consequence 😅

I think the comments above and below the if are swapped.

🙈

Also is this check only relevant when the condition table happens to contain the same value as the SBML default? Or is there another case where this becomes relevant? IMO fine to skip this check if it's just about cases where the user coincidentally duplicates the SBML default into the condition table -- if they have redundant information in the table then redundant initial assignments can be the consequence 😅

Yes and no. Imagine you have 100 experiments, and only one of them changes the initial value of A. This would result in A = Piecewise((default, exp1), (default, exp2), ..., (default, exp99) , (some_non_default, exp100)), unless sympy will auto-simplify that.

Since it's not that complicated I prefer keeping that here to produce easier-to-digest models.

Ah nice, thanks 👍

dilpath · 2025-10-02T13:51:58Z

petab/v2/converters.py

+                    if species_change := next(
+                        (c for c in changes if c.target_id == species_id), None
+                    ):


Fine as is, but I use more-itertools frequently for one and only.

https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.only

Generally useful, agreed, but not sure if I want to pull in another dependency for that.

dilpath · 2025-10-02T13:54:56Z

petab/v2/converters.py

+                    _add_assignment(
+                        event,
+                        species_id,
+                        # new_conc * new_volume / old_volume
+                        new_conc
+                        * change.target_value
+                        / sp.Symbol(change.target_id),
+                    )


Does SBML ensure that the compartment change and derived concentrations are implemented first in a set of event assignments, before event assignments for the derived concentrations explicitly? And is sp.Symbol(change.target_id) necessarily the old compartment size, if the compartment change is implemented first?

For multiple event assignments of a single event (we don't care about simultaneous events here), the assigned expression is evaluated before any of the assignments.

I'd say there is no explicit order, but the updated concentrations will be computed based on the new expressions for the compartment size (which is kind of the same).

Thanks, looks good 👍

petab/v2/converters.py

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

dweindl added 4 commits October 2, 2025 10:06

Update to changed change semantics

0735774

initial period as initial assignment

839aaff

rename

f01dfb0

..

d524898

dweindl force-pushed the experiments2events branch from 17a3764 to d524898 Compare October 2, 2025 08:14

dweindl marked this pull request as ready for review October 2, 2025 08:14

dweindl requested a review from a team as a code owner October 2, 2025 08:14

dweindl linked an issue Oct 2, 2025 that may be closed by this pull request

Update ExperimentsToEventsConverter to changed initialization semantics #425

Closed

dilpath approved these changes Oct 2, 2025

View reviewed changes

dweindl and others added 3 commits October 3, 2025 09:43

Apply suggestions from code review

d8bc4f6

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

..

568e98a

Merge branch 'main' into experiments2events

778935a

dweindl merged commit 6f0891e into PEtab-dev:main Oct 4, 2025
7 checks passed

dweindl deleted the experiments2events branch October 4, 2025 16:38

Update ExperimentsToEventsConverter to changed initialization semantics #443

Update ExperimentsToEventsConverter to changed initialization semantics #443

Uh oh!

Conversation

dweindl commented Sep 30, 2025

Uh oh!

codecov-commenter commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dilpath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Update `ExperimentsToEventsConverter` to changed initialization semantics #443

Update `ExperimentsToEventsConverter` to changed initialization semantics #443

codecov-commenter commented Sep 30, 2025 •

edited

Loading