Skip to content
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

GSoC - SBML L3V2 models support with some other bug fixes #48

Merged
merged 67 commits into from
Jul 3, 2020

Conversation

hemilpanchiwala
Copy link
Member

Major changes made in SBSCL (from GSoC start):

  • Before this PR, two PRs have been merged to SBSCL. A complete description of changes made is described in that PRs.
  • Absolute as well as Relative tolerances are now fixed for the simulation of SBML models. This has solved the problems of failing test cases of the SBML Test Suite due to small deviations.
  • Now, SBML models with fbc extensions are simulated using FluxBalanceAnalysis in SBMLTestSuiteTest and SBMLTestRunnerWrapper.
  • Active objective function variable is added to FluxBalanceAnalysis. And now all the fbc models in the SBML Test Suite pass successfully.
  • SBML L3V2 models issues solved:
    • SBML L3V2 models do not have a fast property which will be handled now.
    • SBML L3V2 models now can have constraints, kinetic laws, events, species, rules (assignment rules and rate rules), initial assignments, and any others without math. This has been handled now through this PR in SBSCL.
    • Logical operators are now added to compileDouble in ASTNodeValue.
    • New functions are added to the ASTNodeInterpreter class. (issue Add functions in ASTNodeInterpreter class for calculating node values of SBML L3V2 models #42)
    • Reaction without reactants and products has been solved now. (issue Simulation of models with reaction without reactants and products #41)
    • RELATIONAL_GT, RELATIONAL_GEQ, RELATIONAL_EQ, RELATIONAL_NEQ, RELATIONAL_LEQ, and RELATIONAL_LT are now updated to have multiple children.
    • SBSCL now supports rateOf csymbol that is newly added in SBML L3V2.
    • New variables have been added to RateRuleValue and RuleValue classes.
    • A new array changeRate has been added to SBMLinterpreter class that keeps the value of the derivatives at the current time.
    • constantHash has also been added to SBMLinterpreter that has key as variables and values are boolean (true if constant else false).
    • After making this changes, about 67 SBML L3V2 tests fail excluding the comp tests from the SBML Test Suite.
  • Many other small issues have been resolved in this PR.

This PR closes #38, closes #41, closes #42, closes #43.
More information about changes made till now in SBSCL can be found at my blog posts [Link].

@matthiaskoenig matthiaskoenig self-requested a review June 28, 2020 22:00
@hemilpanchiwala hemilpanchiwala linked an issue Jun 29, 2020 that may be closed by this pull request
6 tasks
@hemilpanchiwala
Copy link
Member Author

hemilpanchiwala commented Jul 1, 2020

Why did subclassing not work? I.e. only overwriting the solve method?

@matthiaskoenig,
It did not work as the new instance of GlpkSolver was made in the solve() method explicitly. We did not have any reference to that class in SBSCL.

Here, the thing is going on as follows:
We make an object of LinearProgramSolver (which is an interface) that is implemented by GLPKSolver class where the solve() method implementation is found. In this solve() method, scpsolver creates an instance of the GlpkSolver class which was creating all the problems and so making that instance (of GlpkSolver) null using deleteProb() solves the issue. But this is all in scpsolver so I made a copy of the GLPKSolver class with making the instance of GlpkSolver null on completing its usage. Once scpsolver makes this change then we can remove this class.

Note: GLPKSolver and GlpkSolver are different classes in scpsolver.

@hemilpanchiwala hemilpanchiwala linked an issue Jul 2, 2020 that may be closed by this pull request
Copy link
Collaborator

@matthiaskoenig matthiaskoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requested.
Please add a FluxBalanceAnalysis test which:

  • takes the ecoli core model, simulates a FBA, compares objective; currently a simple test case for FBA outside of the SBMLTestSuite is missing

if (solver instanceof AbstractDESSolver) {
((AbstractDESSolver) solver).setIncludeIntermediates(false);
}
String fileName = "";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally one initializes with null. Empty string is always a very risky choice. Change to null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the code!

* @return
*/
public String getActObjFunc() {
return actObjFunc;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name is activeObjective. Always write words out. There is code completion which makes larger names easy to use. Also rename functions to getActiveObjective.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

@matthiaskoenig
Copy link
Collaborator

Will merge this as soon as the minor changes & the test case are there.

@hemilpanchiwala
Copy link
Member Author

Please add a FluxBalanceAnalysis test which:

  • takes the ecoli core model, simulates a FBA, compares objective; currently a simple test case for FBA outside of the SBMLTestSuite is missing

@matthiaskoenig, I have updated the changes. And FBA test was there but it was named CobraSolverTest and was ignored by @ignore tag. I have renamed it to FluxBalanceAnalysisTest and also removed the ignore tag.

@draeger draeger merged commit c0d1077 into draeger-lab:master Jul 3, 2020
Copy link
Collaborator

@shalinshah1993 shalinshah1993 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay here, congratulations @hemilpanchiwala
This is fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment