Option to convert model assignments to experiments in petab1to2 by BSnelling · Pull Request #527 · PEtab-dev/libpetab-python

BSnelling · 2026-10-07T10:45:14Z

The option identifies piecewise time dependent assignments from the model, converts them into rows in the experiments/condition tables in the petab v1 problem, and removes those assignments from the model.

codecov-commenter · 2026-10-07T10:47:38Z

Codecov Report

❌ Patch coverage is 92.30769% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.43%. Comparing base (e46ef71) to head (5720c99).

Files with missing lines	Patch %	Lines
petab/v2/converters.py	92.30%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   77.16%   77.43%   +0.26%     
==========================================
  Files          67       67              
  Lines        7568     7670     +102     
  Branches     1349     1374      +25     
==========================================
+ Hits         5840     5939      +99     
  Misses       1242     1242              
- Partials      486      489       +3

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

dilpath

I did not receive a notification to review this somehow...

Thanks! I think it's a useful addition but I am a bit unsure whether some cases are handled currently.

dilpath · 2026-10-08T20:25:06Z

+        replaced_columns |= {str(s) for s in expr.free_symbols}
+
+        for sim_id, preeq_id in pairs:
+            t_sim = float(t_switch.subs(overrides[sim_id]))


Is it possible that an error arises here (or downstream) if t_switch has species, i.e. t_switch cannot be solved for a specific float time but might still be some sp.Expr involving species?

The overrides could also be problematic. If a species appears in a v1 condition table column, then its initial condition is being set there. Hence, the initial value of a species might be substituted here via overrides I think, which could silently cause an error because species should rather raise an error.

More specifically, a t_switch that contains species cannot be solved in general for specific time(s), before simulation.

I've added a check that the switch can be resolved to a number and throw if not. I think that should also cover the case where t_switch contains species?

Thanks. I am wondering if overrides can contain an "override" for a species. If so, that override (which represents the species at the initial condition) will be implemented into t_switch. This would be a difference to the SBML model, which would implement the dynamic value of the species into t_switch over the course of the simulation.

That sounds like it might have to be highlighted as a case we can't support with this conversion?

Agreed, was just pointing out that the case should be handled with some NotImplementedError.

Not sure if this is too strict but do you think this covers it? 1fc1744

Thanks! I think it's good like this -- if someone complains, then we can broaden the implementation at that point.

dilpath · 2026-10-08T20:31:57Z

        shutil.copy(str(src), str(dest))


+def _assignments_to_periods(


I think this replaces SBML piecewise terms with their values from the simulation condition. Maybe this can change the simulation (and thereby likelihood). For example, I think these piecewises would have triggered during preequilibration in v1. If we move them to the experiment table, then they will no longer trigger during preequilibration.

So maybe the only safe way to implement this is to only perform _assignments_to_periods for v1 simulations that do not involve preequilibration. But this seems complicated...

I'm not sure I understand the use case, but now that I've added the real number check around switching times, I think a piecewise triggered during preequilibration would raise a NotImplemented. I could make it more explicit and not support problems with prequilibration at all if you think that would be safer.

I also do not understand the use case -- but when events are in the SBML model, they are triggered during both preequilibration and simulation. However, when your code converts these to v2 experiments, they are no longer "triggered" during preequilibration. One would need to hack them in somehow so that they also trigger during preequilibration...

Thanks for @dweindl for clarifying elsewhere -- for this case, could you add a warning in bold to the docstring that, in the converted problem, the changes are not implemented during preequilibration? And also detect whether the PEtab v1 problem has preeq and if so, issue a warning about this.

Added warning in the docstring and in the code.

dilpath · 2026-10-08T20:43:41Z

+            elif {str(s) for s in values[t_sim].free_symbols} <= parameter_ids:
+                # the model already has the pre-switch value, and the switch
+                #  is valid as first period (only parameter table symbols)
+                del values[0.0]


I am not sure I follow the logic here but could there be an issue if, for example, the model contains an SBML event that changes the value of a PEtab parameter at some timepoint $t: 0 < t < t_{sim}$? @dweindl ?

I think that could be an issue. Assignment rules would get re-evaluated after the event triggerd but the condition wouldn't.

I could protect against that by iterating through the event assignments and checking that no petab parameters in there appear in the converted piecewise assignments?

Nevermind, v2 explicitly excludes this case: the parameter table must contain truly constant parameters (e.g. the parameters cannot be changed by an SBML event).

Hence, throw an error if it looks like you are about to implement an estimated parameter in the condition table (which is anyway invalid, so alternatively you could detect the issue by validating your generated PEtab problem --this approach is also fine for me). In any case, please add a small test for this.

Added a test with this case and a linting step at the end of the function.

dweindl · 2026-10-09T09:38:59Z

I see how this can be useful. However, I am wondering if it wouldn't be substantially more maintainable if we would separate that from the v1->v2 conversion, and do that as an independent post-conversion pass. In that case, the input would be a v2 problem with a couple of additional things to handle, but those could be rejected (non-zero starting time, non-sbml models, ...).

BSnelling · 2026-10-12T10:52:32Z

Thanks both! I've moved the feature into its own function as @dweindl suggested and added safety guards around certain cases. For now I'm keeping the scope pretty narrow, just piecewise assignments that are resolvable to a specific time are supported, other cases raise NotImplemented.

dilpath

Thanks! Happy to review again after the discussed changes. As discussed, I would suggest we explicitly support the following cases only:

A single piecewise function with exactly 3 arguments, where the 2nd argument is an inequality that can be re-arranged explicitly into either of t > ... or t >= ..., and error on anything else.
The first and third arguments should be some numeric value or anything else that is a valid targetValue in the v2 condition table (also check this -- linting the problem afterwards is fine for me).
If there is preequilibration, then there will be the discussed breaking change to warn about.

Also write this scope into the docstring of the function.

dilpath · 2026-10-13T12:54:56Z

+
+                t_end = periods[i + 1].time if i + 1 < len(periods) else sp.oo
+                t = t_switch.subs(subs)
+                if not (t.is_number and t.is_real):


I would additionally suggest t.is_finite. If t evaluates to negative infinity then maybe preequilibration is erroneously implemented 🤷

dilpath · 2026-10-13T12:57:32Z

+        replaced_columns |= {str(s) for s in expr.free_symbols}
+
+        for sim_id, preeq_id in pairs:
+            t_sim = float(t_switch.subs(overrides[sim_id]))


Thanks. I am wondering if overrides can contain an "override" for a species. If so, that override (which represents the species at the initial condition) will be implemented into t_switch. This would be a difference to the SBML model, which would implement the dynamic value of the species into t_switch over the course of the simulation.

dilpath · 2026-10-13T13:04:49Z

        shutil.copy(str(src), str(dest))


+def _assignments_to_periods(


I also do not understand the use case -- but when events are in the SBML model, they are triggered during both preequilibration and simulation. However, when your code converts these to v2 experiments, they are no longer "triggered" during preequilibration. One would need to hack them in somehow so that they also trigger during preequilibration...

Thanks for @dweindl for clarifying elsewhere -- for this case, could you add a warning in bold to the docstring that, in the converted problem, the changes are not implemented during preequilibration? And also detect whether the PEtab v1 problem has preeq and if so, issue a warning about this.

dilpath · 2026-10-13T13:12:31Z

+            elif {str(s) for s in values[t_sim].free_symbols} <= parameter_ids:
+                # the model already has the pre-switch value, and the switch
+                #  is valid as first period (only parameter table symbols)
+                del values[0.0]


Nevermind, v2 explicitly excludes this case: the parameter table must contain truly constant parameters (e.g. the parameters cannot be changed by an SBML event).

Hence, throw an error if it looks like you are about to implement an estimated parameter in the condition table (which is anyway invalid, so alternatively you could detect the issue by validating your generated PEtab problem --this approach is also fine for me). In any case, please add a small test for this.

dilpath · 2026-10-13T13:42:22Z

+        "piecewise(0, time < 10, 1) + piecewise(0, time < 20, 1)",
+        "piecewise(0, time < 10, 1, time < 20, 2)",


Is this case handled? e.g. time < 10 || time > 15?

This will now throw a NotImplemented because it is not a single inequality.

dilpath

Thanks! Fine to merge now I think

…nverter

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

BSnelling requested a review from a team as a code owner October 7, 2026 10:45

dilpath requested changes Oct 8, 2026

View reviewed changes

BSnelling force-pushed the bes/models_to_exps branch from 9dc1783 to 3714d62 Compare October 12, 2026 10:44

BSnelling requested a review from dilpath October 12, 2026 11:01

dilpath requested changes Oct 13, 2026

View reviewed changes

BSnelling requested a review from dilpath October 14, 2026 08:47

dilpath approved these changes Oct 14, 2026

View reviewed changes

Comment thread petab/v2/converters.py Outdated

Comment thread petab/v2/converters.py

Comment thread tests/v2/test_conversion.py Outdated

BSnelling and others added 12 commits October 14, 2026 14:23

add option to convert model assignments to experiments in v1 to v2 co…

80817ef

…nverter

make assignments to exps a separate function acting on v2 problem

85f4b6b

throw not implemented for multi-piecewise cases

2adf87a

guard around t switch unresolveable

09ec291

fix up rebase

cb4d06b

fix to support both switch directions

2f2c2aa

review feedback on preeq and linting

047c688

trim scope of conversion

9a43cca

fix ruff

2900a70

not implemented for variable model params

60f8057

Apply batched suggestions from code review

54cd742

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

updates to docstrings

5720c99

BSnelling force-pushed the bes/models_to_exps branch from 92af18b to 5720c99 Compare October 14, 2026 13:23

BSnelling merged commit 1c3d7b9 into main Oct 14, 2026
13 checks passed

BSnelling deleted the bes/models_to_exps branch October 14, 2026 13:29

		shutil.copy(str(src), str(dest))


		def _assignments_to_periods(

		"piecewise(0, time < 10, 1) + piecewise(0, time < 20, 1)",
		"piecewise(0, time < 10, 1, time < 20, 2)",

Conversation

BSnelling commented Oct 7, 2026

Uh oh!

codecov-commenter commented Oct 7, 2026 • 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!

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!

dweindl commented Oct 9, 2026

Uh oh!

BSnelling commented Oct 12, 2026

Uh oh!

dilpath left a comment

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!

dilpath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 7, 2026 •

edited

Loading