Refactor rateOf handling #3036
Merged
Refactor rateOf handling #3036
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3036 +/- ##
==========================================
- Coverage 77.41% 77.38% -0.03%
==========================================
Files 311 311
Lines 20594 20597 +3
Branches 1500 1499 -1
==========================================
- Hits 15942 15940 -2
- Misses 4642 4647 +5
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dweindl
force-pushed
the
refactor_rateof
branch
3 times, most recently
from
5a5a64f to
0eea980
Compare
GitHub Action runners run out of disk space when installing petab-sciml with all its huge dependencies. Don't install that for now. So far, it's not used anywhere for the documentation build as far as I can see. This won't prevent enabling intersphinx later on.
FFroehlich
approved these changes
Nov 15, 2025
| if rate_ofs := ae.get_val().find(rate_of_func): | ||
| # Not implemented | ||
| raise SBMLException( | ||
| "rateOf() is not supported in algebraic equations." |
Member
There was a problem hiding this comment.
include name of ae?, why is this necessary in given is_ode check below?
Member
Author
There was a problem hiding this comment.
include name of ae?
👍
why is this necessary in given
is_odecheck below?
Currently, no substitution is even attempted for any ae. Therefore, that check will never trigger. Will add a comment.
Pull rateOf-handling out of DEModel and keep it along other SBML processing where it belongs. This is easier to follow and prevents some lingering issues with the old approach due to the xdot / w interdependencies. This also handles rateOf expressions in some additional, previously unsupported places like event assignments.
dweindl
force-pushed
the
refactor_rateof
branch
from
0eea980 to
e7b4896
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 15, 2025
* doc: Skip petab-sciml for now GitHub Action runners run out of disk space when installing petab-sciml with all its huge dependencies. Don't install that for now. So far, it's not used anywhere for the documentation build as far as I can see. This won't prevent enabling intersphinx later on. * Refactor rateOf handling Pull rateOf-handling out of DEModel and keep it along other SBML processing where it belongs. This is easier to follow and prevents some lingering issues with the old approach due to the xdot / w interdependencies. This also handles rateOf expressions in some additional, previously unsupported places like event assignments.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
dweindl
added a commit
to dweindl/AMICI
that referenced
this pull request
Nov 15, 2025
* doc: Skip petab-sciml for now GitHub Action runners run out of disk space when installing petab-sciml with all its huge dependencies. Don't install that for now. So far, it's not used anywhere for the documentation build as far as I can see. This won't prevent enabling intersphinx later on. * Refactor rateOf handling Pull rateOf-handling out of DEModel and keep it along other SBML processing where it belongs. This is easier to follow and prevents some lingering issues with the old approach due to the xdot / w interdependencies. This also handles rateOf expressions in some additional, previously unsupported places like event assignments.
dweindl
added a commit
to dweindl/AMICI
that referenced
this pull request
Nov 17, 2025
With AMICI-dev#3036, the ordering of symbols and expressions has changed. This is not a problem per se, but would require recreating the test oracles for the models in `models/`. Therefore, don't do any unnecessary reordering.
Merged
dweindl
added a commit
to dweindl/AMICI
that referenced
this pull request
Nov 17, 2025
With AMICI-dev#3036, the ordering of symbols and expressions has changed. This is not a problem per se, but would require recreating the test oracles for the models in `models/`. Therefore, don't do any unnecessary reordering.
dweindl
added a commit
to dweindl/AMICI
that referenced
this pull request
Nov 18, 2025
With AMICI-dev#3036, the ordering of `w` symbols and expressions has changed. This is not a problem per se, but would require recreating the test oracles for the models in `models/`. Therefore, don't do any unnecessary reordering.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull rateOf-handling out of DEModel and keep it along other SBML processing where it belongs.
This is easier to follow and prevents some lingering issues with the old approach due to the xdot / w interdependencies.
This also handles rateOf expressions in some additional, previously unsupported places like event assignments.
I think this also fixes a potential bug in the
wreordering in_process_hybridizationthat might otherwise trigger in combination with conservations laws, because they might end up at the beginning ofw.A minor downside is that in xdot rateOf expressions are replaced by the respective xdot equations where previously xdot symbols were used, but I prefer the reduced complexity. If this becomes a performance problem, this can easily be addressed by shifting rateOf into
wby adding an extra assignment rule to the model.