Resolves issues of SBMLTestSuite and FBA #40

hemilpanchiwala · 2020-06-03T18:13:32Z

Major changes from this PR:

Adds a running wrapper for the SBML Test Runner that allows to check the results through the plot and compare the distance plots.
Adds testing by CompSimulator for the comp models in SBMLTestSuiteTest
Adds three new distance metrics in SBSCL.
- Maximum Absolute Distance
- Maximum Relative Distance
- Maximum Divergence Tolerance - The metric used by SBML Test Runner
Adds the newly added tests from SBML Test Suite.
Updates the INSTALL.md section for the SBML Test Runner.
Moves all the script files under the src/test/scripts folder. Updated the scripts files.
CobraSolver is refactored to FluxBalancedAnalysis.
Solves the issues with the FluxBalanceAnalysis so that all the FBC tests from SBML Test Suite now runs perfectly.
Fixes some other minor issues and some enhancement in the code

…into testrunner

draeger

As a general recommendation, I'd suggest making use of the templates for code style from the JSBML project at https://github.com/sbmlteam/jsbml/tree/master/dev/. They can really help to automatically insert blank spaces, curly braces, and so forth, where forgotten.

Further comments were given in the code.

draeger · 2020-06-03T18:17:30Z

src/main/java/org/simulator/math/QualityMeasure.java

+   * @param x
+   * @param expected
+   * @return maxAbsDistances the list of maximum absolute distances in the blocks
+   */
  public ArrayList<Double> getMaxAbsDistances(MultiTable x, MultiTable expected) {


Please do not declare unnecessarily specific return types for methods and also declare variables with the least specific type. In this concrete case, please use List<Double> to initialize the variable for the distances and also return a List<Double>. You can still use an ArrayList<Double> to initialize the variable, but not as return type. This applies to other positions as well.

draeger · 2020-06-03T18:20:48Z

src/test/download_bigg_models.sh

-cd resources/bigg
+cd TEST_DIR
+mkdir -p $TEST_DIR/resources/bigg
+cd $TEST_DIR/resources/bigg
 wget https://github.com/matthiaskoenig/bigg-models-fba/raw/master/models/bigg_models_v1.5.tar.gz


@matthiaskoenig Not clear to me why we don't download the BiGG models directly from BiGG. Doesn't this detour introduce the problem that we need to keep two places in sync with BiGG Models Database?

draeger · 2020-06-03T18:26:19Z

src/test/java/org/simulator/sbml/SBMLTestSuiteTest.java

+                        // compute distance
+                        QualityMeasure distance = new RelativeEuclideanDistance();
+                        double dist = distance.distance(solution, inputData);
+                        Assert.assertTrue(dist <= 0.2);


I'd recommend to always indicate the data type also for literal numbers, here, for instance, 0.2d so that the compiler knows it is a double value. If you do it always, you will not forget it in situations where it can make a difference (e.g., when dividing integer numbers).

@draeger, this has been removed from the code now. The distance has been now updated to the MaxDivergenceTolerance.

draeger · 2020-06-03T18:27:43Z

src/main/java/org/simulator/math/QualityMeasure.java

   */
-  public ArrayList<Double> getColumnDistances(MultiTable x, MultiTable expected) {
-    ArrayList<Double> distances= new ArrayList<Double>();
+  public HashMap<String, Double> getMaxAbsDistances(MultiTable x, MultiTable expected) {


Also here, please do not return HashMap<String, Double> but Map<String, Double> to make it easier to use a different implementation later on.

@draeger, Updated this in the latest commit.

hemilpanchiwala · 2020-06-03T18:53:01Z

As a general recommendation, I'd suggest making use of the templates for code style from the JSBML project at https://github.com/sbmlteam/jsbml/tree/master/dev/. They can really help to automatically insert blank spaces, curly braces, and so forth, where forgotten.

Further comments were given in the code.

@draeger, I can just see their recommendations but can't add the templates as I am working on IntelliJ (even prefer), and the templates are only present for Eclipse.

draeger · 2020-06-03T18:59:29Z

As a general recommendation, I'd suggest making use of the templates for code style from the JSBML project at https://github.com/sbmlteam/jsbml/tree/master/dev/. They can really help to automatically insert blank spaces, curly braces, and so forth, where forgotten.
Further comments were given in the code.

@draeger, I can just see their recommendations but can't add the templates as I am working on IntelliJ (even prefer), and the templates are only present for Eclipse.

Ok; so IntelliJ doesn't have an import for such files or doesn't come with comparable functions?

hemilpanchiwala · 2020-06-03T19:04:21Z

As a general recommendation, I'd suggest making use of the templates for code style from the JSBML project at https://github.com/sbmlteam/jsbml/tree/master/dev/. They can really help to automatically insert blank spaces, curly braces, and so forth, where forgotten.
Further comments were given in the code.

@draeger, I can just see their recommendations but can't add the templates as I am working on IntelliJ (even prefer), and the templates are only present for Eclipse.

Ok; so IntelliJ doesn't have an import for such files or doesn't come with comparable functions?

I can just format the code as per the Java code style which is provided in IntelliJ given at this link.

matthiaskoenig · 2020-06-03T20:57:33Z

.travis.yml

   # get BiGG models for testing
   - source ./download_bigg_models.sh
   # get sbml-test-case models for testing
   - source ./download_sbml-test-suite.sh
-   - cd ../..
+   - cd ../../..

 script:
 - mvn clean install


add newline at end of file (I even get the warning here)

matthiaskoenig · 2020-06-03T20:58:56Z

INSTALL.md

+mvn clean install -DskipTests
+```
+
+The tests require additional resources which have to be downloaded. These include the sbml-testsuite and the BiGG models.


Please add links to the project pages (i.e. the git repository for the BiGG models and for the sbml-testsuite)! We are using these resources and should be linked in the documentation.

matthiaskoenig · 2020-06-03T21:02:18Z

.travis.yml

@@ -5,12 +5,12 @@ jdk:
  - oraclejdk11

 install:
-   - cd src/test
+   - cd src/test/scripts


Don't do the cd and back here!
Just directly call the scripts as mentioned in the installation instructions

source ./src/test/scripts/download_bigg_models.sh source ./src/test/scripts/download_sbml-test-suite.sh

matthiaskoenig · 2020-06-03T21:02:45Z

INSTALL.md

+The tests require additional resources which have to be downloaded. These include the sbml-testsuite and the BiGG models.
+```
+source ./src/test/download_bigg_models.sh
+source ./src/test/download_sbml-test-suite.sh


missing the scripts directory

matthiaskoenig · 2020-06-03T21:04:21Z

INSTALL.md

+```
+
+## SBML Test Runner
+The SBML Test Runner is a standalone desktop application with a graphical user interface (GUI) for controlling SBML-compatible applications and making them run test cases from the [SBML Test Suite](http://sbml.org/Software/SBML_Test_Suite). It is written in Java with the Eclipse SWT widget toolkit and can be used on macOS, Linux and Windows.


Delete: It is written in Java with the Eclipse SWT widget toolkit and can be used on macOS, Linux and Windows.

matthiaskoenig · 2020-06-03T21:17:45Z

src/main/java/org/simulator/examples/SBMLTestSuiteRunnerWrapper.java

+
+        MultiTable solution;
+
+        if (model.getExtension(CompConstants.shortLabel) == null){


You do not set the tolerances anywhere for the simulation. Please set the tolerances explicitly! Only then it is possible to check the solutions correctly.

matthiaskoenig · 2020-06-03T21:19:48Z

src/main/java/org/simulator/fba/FluxBalanceAnalysis.java

+	 * @return the epsilon value
+	 */
+	public double getEpsilon() {
+		return EPS;


Please make this lowercase and call it epsilon or eps.

@matthiaskoenig is here referring to the name of the variable, not the name of the method.

matthiaskoenig · 2020-06-03T21:20:57Z

src/main/java/org/simulator/io/CSVImporter.java

-      for (String head : header) {
-        if (!head.equalsIgnoreCase(data.getTimeName())) {
-          newHead[i++] = head.trim();
+      for (int p = 1; p < header.length; p++) {


Please add a comment why this loop is starting at 1. What are you ignoring?

@matthiaskoenig, the time column has to be ignored for the newHead array. So, its started at index 1.

This was the main reason why the travis ignored the SBMLTestSuiteTest file.

Please just add a comment. Here, it can be as simple as adding // we are starting at 1 because... in the line over the loop.

@draeger, this is commented above at line 100 in this file: // exclude time column.

matthiaskoenig · 2020-06-03T21:24:26Z

src/main/java/org/simulator/math/MaxDivergenceTolerance.java

+    }
+
+    /* (non-Javadoc)
+     * @see org.sbml.simulator.math.Distance#distance(java.lang.Iterable, java.lang.Iterable, double)


Document this better ! Please describe what the distance is and state the used formula in the comment. This is the core comparison function metric used. This must be documented so that others understand what is done here.

Added the comments properly in the initial description of the class.

matthiaskoenig · 2020-06-03T21:29:40Z

src/main/java/org/simulator/math/MaxDivergenceTolerance.java

+     * @see org.sbml.simulator.math.Distance#distance(java.lang.Iterable, java.lang.Iterable, double)
+     */
+    @Override
+    public double distance(Column x, Column y, double defaultValue) {


Also did not see any test code for this.

Adding in the next commit!

Added the tests for MaxDivergenceTolerance class

hemilpanchiwala · 2020-06-04T08:50:43Z

change name of method. This is very unspecific. Use something like addBoundsEpsilon which states what the method is doing. updateBounds could do anything with the bounds. Also probably a good idea to define the eps locally here (because it is not used anywhere else).

@matthiaskoenig, but we have given get and set methods for the eps. So, I don't think we should declare it locally in the method.

draeger · 2020-06-05T08:19:58Z

src/main/java/org/simulator/examples/SBMLTestSuiteRunnerWrapper.java

+        String filePath = dirPath + File.separator + currentCase + File.separator + currentCase + "-sbml-l" + level + 'v' + version + ".xml";
+        String settingsPath = dirPath + File.separator + currentCase + File.separator + currentCase + "-settings.txt";
+        String outputFilePath = outputDirPath + File.separator + currentCase + ".csv";
+        String resultsPath = dirPath + File.separator + currentCase + File.separator + currentCase + "-results.csv";


Did anybody test this under Windows? To my knowledge, internally Java always uses / as separator in paths and the virtual machine translates it to the operating system.

Just to add: I think this will work, but I also think it is not needed (without having it tested).

Yes, I just saw that when we give /, then Java internally handles things to translate to platform-specific code. But File.separator can be a good way to show users what will make sense in their OS, rather than what makes sense to Java.

draeger · 2020-06-05T08:24:37Z

src/main/java/org/simulator/examples/SBMLTestSuiteRunnerWrapper.java

+        double absolute = (!properties.getProperty("absolute").isEmpty()) ? Double.parseDouble(properties.getProperty("absolute")) : 0d;
+        double relative = (!properties.getProperty("relative").isEmpty()) ? Double.parseDouble(properties.getProperty("relative")) : 0d;


It would be preferable to declare String literals in the form public static final String ABSOLUTE = "absolute" and then to reuse these constants where needed. This reduces maintenance effort and possible errors caused by typos.

Another possibility is creating an internal enum such as:

public enum SettingsKeywords { absolute; ... }

that can be uses similarly.

Updated by adding final variables for Strings.

hemilpanchiwala and others added 30 commits May 22, 2020 22:58

resolved small errors occured while running SBMLTestSuite tests

55df3d7

updated instructions for download of test files

bb4d201

excluding the 1592 case in SBMLTestSuite

300c1da

Skipping draeger-lab#39 test for now

734c3c3

Merge branch 'testrunner' of https://github.com/hemilpanchiwala/SBSCL …

a6619d0

…into testrunner

better exclusion of test

f05924b

added maxAbsDistance method

9286a18

added CompSimulator for tests of comp models in SBMLTestSuiteTest.java

b477124

resolve small issue

dab6436

add MaxAbsDistance class and update the SBMLTestSuite check

1dfd51f

add RelMaxDistance in SBSCL

f1c1423

add wrapper for SBML Test Runner

3591270

bugfix for relative paths

ba828ea

update to add columns requested by test suite in csv file

e63dd74

add SBML Test Runner section in INSTALL.md

c76a59b

add tests for maxAbsDistance and maxRelDistance

769648a

resolve small issue

ea37c6d

minor fix

542abed

small bugfix

8e53737

add distance plot criteria link for SBML Test Runner

ceb057e

add CompSimulator for comp tests in SBML Test Runner wrapper

c819b4c

update download_sbml-test-suite script to download updated test cases

df231e8

minor add to download_sbml-test-suite.sh

30b3b88

remove NEWS.md file from SBML Test Suite as it seems broken

6c86cbc

add scripts folder under src/test folder

bc20db6

update script paths in .travis.yml

73cc11c

add new distance metric for SBMLTestSuiteTest - MaxDivergenceTolerance

b7010f2

minor fix

2f7421a

enhanced code in some classes

8a299f5

resolves issue draeger-lab#38

c110042

hemilpanchiwala added 7 commits June 2, 2020 23:32

refactored CobraSolver to FluxBalanceAnalysis solving issue #4

f69cb3d

minor fix

d90f374

resolve issue #5 and resolve fbc namespace issue

ac43ac0

update getting solution of FBA as hashmap instead of array

ddf5482

update requested changes in commit ac43ac0

59e6bf4

change HashMap to Map to make less specific

6053292

update requested changes in commit 59e6bf4

c327f01

hemilpanchiwala requested review from draeger and matthiaskoenig June 3, 2020 18:15

draeger requested changes Jun 3, 2020

View reviewed changes

change ArrayList to List to make code less specific

e95aff6

matthiaskoenig requested changes Jun 3, 2020

View reviewed changes

hemilpanchiwala added 3 commits June 4, 2020 19:02

update requested changes

e71be5d

add comments for eps in FluxBalanceAnalysis

fbecdc7

refactored the addBoundsEpsilon method to adjustBoundNumerics

5f1f6d4

draeger reviewed Jun 5, 2020

View reviewed changes

add variables for string literals

c34d88a

draeger merged commit f4da6ab into draeger-lab:master Jun 5, 2020

hemilpanchiwala mentioned this pull request Jun 18, 2020

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

Merged

hemilpanchiwala added the gsoc label Jun 18, 2020


		MultiTable solution;

		if (model.getExtension(CompConstants.shortLabel) == null){

		double absolute = (!properties.getProperty("absolute").isEmpty()) ? Double.parseDouble(properties.getProperty("absolute")) : 0d;
		double relative = (!properties.getProperty("relative").isEmpty()) ? Double.parseDouble(properties.getProperty("relative")) : 0d;

Resolves issues of SBMLTestSuite and FBA #40

Resolves issues of SBMLTestSuite and FBA #40

Conversation

hemilpanchiwala commented Jun 3, 2020

draeger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemilpanchiwala commented Jun 3, 2020 • edited Loading

draeger commented Jun 3, 2020

hemilpanchiwala commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemilpanchiwala commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hemilpanchiwala commented Jun 3, 2020 •

edited

Loading