-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Rule-handling in PEtab import #1753
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1753 +/- ##
===========================================
- Coverage 73.59% 65.82% -7.77%
===========================================
Files 71 30 -41
Lines 12053 4337 -7716
===========================================
- Hits 8870 2855 -6015
+ Misses 3183 1482 -1701
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@dweindl Thanks for the work on the issue. As I understand this fixes the rate rule issue. Would it be possible to merge this in develop? |
@matthiaskoenig : Will try to do this week. Needs a new https://github.com/PEtab-dev/libpetab-python release first. |
RateRules were previously ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
value = get_species_initial( | ||
petab_problem.sbml_model.getSpecies(species_id) | ||
) | ||
element = petab_problem.sbml_model.getElementBySId(element_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this doesn't account for InitialAssignments right? (was already the case for species before, but I am not sure thats correct in the first place)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Good catch. Should add a test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -86,7 +86,8 @@ def get_fixed_parameters( | |||
# remove overridden parameters (`object`-type columns) | |||
fixed_parameters = [p for p in fixed_parameters | |||
if condition_df[p].dtype != 'O' | |||
and sbml_model.getParameter(p) is not None] | |||
and sbml_model.getParameter(p) is not None | |||
and sbml_model.getRuleByVariable(p) is None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uuuuh, so what happens with rules here that don't have a RateRule qualifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about whether these parameters should be considered as AMICI-constant-parameters. Any parameters that are rule targets cannot be constants. Or am I missing something?
Co-authored-by: Fabian Fröhlich <fabian_froehlich@hms.harvard.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
SonarCloud Quality Gate failed. |
@matthiaskoenig : now available on pypi - |
RateRules-targets in the condition table were previously ignored.
Closes #1750
Also fixes a bug where InitialAssignments were not honored for elements that occurred as column in the condition table and were set to
NaN
(i.e. "use model value").