Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
+ Coverage 76.15% 76.70% +0.54%
==========================================
Files 67 67
Lines 7528 7576 +48
Branches 1342 1351 +9
==========================================
+ Hits 5733 5811 +78
+ Misses 1294 1267 -27
+ Partials 501 498 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dilpath
left a comment
dilpath
left a comment
There was a problem hiding this comment.
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.
| (pw,) = pws | ||
| (time,) = pw.atoms(TimeSymbol) | ||
| (before, switch), _ = pw.args | ||
| t_switch = sp.solve(switch.lhs - switch.rhs, time)[0] |
There was a problem hiding this comment.
Does _assignments_to_periods work overall for both e.g. t>5 and 5>t?
| pws = [pw for pw in expr.atoms(sp.Piecewise) if pw.has(TimeSymbol)] | ||
| if not pws: | ||
| continue | ||
| (pw,) = pws |
There was a problem hiding this comment.
Does this assume there is only one piecewise function in the assignment? Will >1 piecewise cause an error here? If so, could add an error message so it's properly handled.
Also are piecewises with >2 arguments (i.e. multiple pieces) handled?
| 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])) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| shutil.copy(str(src), str(dest)) | ||
|
|
||
|
|
||
| def _assignments_to_periods( |
There was a problem hiding this comment.
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...
| 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] |
There was a problem hiding this comment.
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
|
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, ...). |
This branch has not been deployed
5 participants
The option identifies
piecewisetime 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.