Fixes regression caused by #387 (#389)

* The default values changed in the commit (ba1065c) breaks example such
as
https://github.com/BhallaLab/moose-examples/blob/09ca09e68f163a0920653a9400214aff10b6ba16/snippets/rxdFuncDiffusion.py
. The sparse matrix has upper limit of number of rows and columns to
20,000. In the example linked, new values makes it impossible to run the
example as is.

To overcome this, we notify the user whenever this limit is reached and
ask them to set `diffLength` before setting up the `x1`.

On top of this, we can also revert default values to original larger
values. Not sure why these values were changed to the scale of 1e-6?
Unless the reason is known, I am not going to change current values. The
above linked example  now works with this commit.

* Script which switches solver is now working fine. Forgot to uncomment `unzombifyModel` in `~Stoich` in previous PR #387.

* switch solver script is working fine now. Added to the test suite.

* During zombiefication, deepcopy of parser is expected. Added more tests
and fixed the deepcopy of parser issue.  In Zombiefication, the parser
was not deep copied resulting in erronous values in couple of tests. There
is some memory leak now.

* Prepare a PR now. Test on travis. Some tests might fail with BOOST.
Memory leak is still there.

* 1. Removed copy constructor from moose::Function. It was not used anywhere.
Increased rtol value for a test. Multithreaded solvers gives different
results.
2. Fixed all memory leaks. Probably I don't understand how EXPRTK really
works. using smart pointers with EXPRTK caused memory leak.
3. Log memory leak on travis using valgrind. Install valgrind in docker images as well.

* During zombiefication, deepcopy of parser is expected. Added fix and a
test case.

* Added more tests and fixed the deepcopy of parser issue.
In Zombiefication, the parser was not deep coped resulting in erronous
values in couple of tets. There is some memory leak now (not very large,
8 bytes per variable per expression).

* Prepare a PR now. Test on travis. Some tests might fail with BOOST.
Memory leak is still there.

* removed copy constructor; it was not used anywhere. [skip ci]
Increased rtol value for a test. Multithreaded solvers gives different
results.

* Fixed all memory leaks. Probably I don't understand how EXPRTK really
works. using smart pointers with EXPRTK caused memory leak.  Log memory
leak on travis using valgrind.  Fixed tests. Let the EXPRK object manage
its memory.

* BOOST/GSL are only needed in ksolve directory: tweaked cmake to change
compilation flags on ksolve directory only. Build will be faster for various configurations.
Moved one test to fixme (Vinu's model).

* Updated gitlab pipeline. On a single threaded virtual machine, numThreads was set to 0 in couple
of tests. Fixed that.  Install pyneuroml to the pipeline.  Small leeway in a test.

* std can be close to zero. np.allclose may not handle it properly all the
time.

* boost solver with GSSA may have large variation.

* boost solver with GSSA may have large variation.  Test for the sum of all errors.

* Increased the tolerance a bit more.

Author: dilawar

.ci/travis_build_linux.sh

@@ -9,6 +9,9 @@
 set -e
 set -x
 
+BUILDDIR=_build_travis
+mkdir -p $BUILDDIR
+
 PYTHON2="/usr/bin/python2"
 PYTHON3="/usr/bin/python3"
 
@@ -32,32 +35,36 @@ $PYTHON3 -m compileall -q .
 
 # Python3 with GSL in debug more.
 (
-    mkdir -p _GSL_BUILD_PY3 && cd _GSL_BUILD_PY3 && \
+    mkdir -p $BUILDDIR && cd $BUILDDIR && \
         cmake -DPYTHON_EXECUTABLE=$PYTHON3 \
         -DCMAKE_INSTALL_PREFIX=/usr -DDEBUG=ON ..
-    # Don't run test_long prefix. They take very long time in DEBUG mode.
-    $MAKE && MOOSE_NUM_THREADS=$NPROC ctest -j$NPROC --output-on-failure -E ".*test_long*"
+    $MAKE
+    # Run with valgrind to log any memory leak.
+    valgrind --leak-check=full ./moose.bin -q -u 
+
+    # Run all tests in debug mode.
+    MOOSE_NUM_THREADS=$NPROC ctest -j$NPROC --output-on-failure 
     make install || sudo make install 
     cd /tmp
     $PYTHON3 -c 'import moose;print(moose.__file__);print(moose.version())'
 )
 
-# BOOST and python3
-(
-    mkdir -p _BOOST_BUILD_PY3 && cd _BOOST_BUILD_PY3 && \
-        cmake -DWITH_BOOST_ODE=ON -DPYTHON_EXECUTABLE="$PYTHON3" \
-        -DCMAKE_INSTALL_PREFIX=/usr ..
-    $MAKE && MOOSE_NUM_THREADS=$NPROC ctest -j$NPROC --output-on-failure 
-)
-
 # GSL and python2, failure is allowed
 set +e
 (
-    BUILDDIR=_GSL_PY2
     mkdir -p $BUILDDIR && cd $BUILDDIR && \
         cmake -DPYTHON_EXECUTABLE=$PYTHON2 -DCMAKE_INSTALL_PREFIX=/usr ..
     $MAKE && MOOSE_NUM_THREADS=$NPROC ctest -j$NPROC --output-on-failure
 )
 set -e
 
+
+# BOOST and python3
+(
+    mkdir -p $BUILDDIR && cd $BUILDDIR && \
+        cmake -DWITH_BOOST_ODE=ON -DPYTHON_EXECUTABLE="$PYTHON3" \
+        -DCMAKE_INSTALL_PREFIX=/usr ..
+    $MAKE && MOOSE_NUM_THREADS=$NPROC ctest -j$NPROC --output-on-failure 
+)
+
 echo "All done"

.ci/travis_build_osx.sh

@@ -20,6 +20,8 @@
 set -o nounset                              # Treat unset variables as an error
 set -e
 
+BUILDDIR=_build_travis
+
 NPROC=$(nproc)
 (
     # Make sure not to pick up python from /opt.
@@ -32,14 +34,14 @@ NPROC=$(nproc)
     $PYTHON3 -m pip install python-libsbml --user
     $PYTHON3 -m pip install pyneuroml --user
 
-    mkdir -p _GSL_BUILD && cd _GSL_BUILD \
+    mkdir -p $BUILDDIR && cd $BUILDDIR \
         && cmake -DPYTHON_EXECUTABLE=$PYTHON3 \
         ..
     make pylint -j$NPROC
     make -j$NPROC && MOOSE_NUM_THREAD=$NPROC ctest --output-on-failure -j$NPROC
 
     cd .. # Now with boost.
-    mkdir -p _BOOST_BUILD && cd _BOOST_BUILD \
+    mkdir -p $BUILDDIR && cd $BUILDDIR \
         && cmake -DWITH_BOOST_ODE=ON \
         -DPYTHON_EXECUTABLE=`which python3` ..
 

.ci/travis_prepare_linux.sh

@@ -13,6 +13,7 @@ apt-get install -qq make cmake
 apt-get install -qq python-numpy python-matplotlib python-networkx python-pip
 apt-get install -qq python3-numpy python3-matplotlib python3-networkx python3-pip
 apt-get install -qq python-tk python3-tk
+apt-get install -qq valgrind
 
 # Gsl
 apt-get install -qq libgsl0-dev || apt-get install -qq libgsl-dev

.gitlab-ci.yml

@@ -13,21 +13,8 @@ build:
     - apt -y install python3-numpy python3-matplotlib python3-pip python3-dev
     - apt -y install python-numpy python-matplotlib python-pip python-dev
   script:
-    - python3 setup.py install
     - python3 -m pip install setuptools pip --user --upgrade
-    - python2 setup.py install 
-    - python2 -m pip install setuptools pip --user --upgrade
-    - python3 setup.py sdist
-  artifacts:
-    paths:
-      - mybinary
-      # depending on your build setup it's most likely a good idea to cache outputs to reduce the build time
-      # cache:
-      #   paths:
-      #     - "*.o"
-
-# run tests using the binary built before
-test:
-  stage: test
-  script:
-    - python3 setup.py test
+    - python3 -m pip install pyneuroml --user --upgrade
+    - python3 setup.py build test 
+    - python3 setup.py install --user
+    - python3 -c "import moose; moose.test()"

CMakeLists.txt

@@ -84,11 +84,7 @@ elseif(ENABLE_UNIT_TESTS)
 else()
     message(STATUS "Building for Release/No unit tests.")
     set(CMAKE_BUILD_TYPE Release)
-		add_definitions(-UDO_UNIT_TESTS -O3 -DDISABLE_DEBUG)
-
-    # DO NOT Treat all warnings as errors. With some compilers and newer versions
-    # this often causes headache.
-    # add_definitions(-Werror)
+    add_definitions(-UDO_UNIT_TESTS -O3 -DDISABLE_DEBUG)
 endif()
 
 if(GPROF AND "${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
@@ -160,20 +156,11 @@ if(WITH_GSL)
             "====================================================================\n"
             )
     endif(NOT GSL_FOUND)
-    add_definitions(-DUSE_GSL)
-    # GSL is also used in RNG (whenever applicable), therefore include paths are
-    # top level.
-    include_directories( ${GSL_INCLUDE_DIRS} )
 elseif(WITH_BOOST_ODE)
     find_package(Boost 1.53 REQUIRED)
     find_package(LAPACK REQUIRED)
 endif()
 
-# if boost ode is being used, don't use GSL.
-if(WITH_BOOST_ODE)
-    add_definitions(-DUSE_BOOST_ODE -UUSE_GSL)
-    include_directories(${Boost_INCLUDE_DIRS})
-endif()
 
 # Openmpi
 if(WITH_MPI)

builtins/Function.cpp

@@ -318,24 +318,8 @@ Function::Function():
 {
 }
 
-Function::Function(const Function& f) :
-    valid_(f.valid_),
-    numVar_(f.numVar_),
-    lastValue_(f.lastValue_),
-    rate_(f.rate_),
-    mode_(f.mode_),
-    useTrigger_(f.useTrigger_),
-    doEvalAtReinit_(f.doEvalAtReinit_),
-    t_(f.t_),
-    independent_(f.independent_),
-    xs_(f.xs_),
-    ys_(f.ys_),
-    stoich_(f.stoich_)
-{
-    parser_.LinkVariables(xs_, ys_, &t_);
-}
-
-// Careful: This is a critical function.
+// Careful: This is a critical function. Also since during zombiefication, deep
+// copy is expected. Merely copying the parser won't work.
 Function& Function::operator=(const Function& rhs)
 {
     // protect from self-assignment.
@@ -351,9 +335,23 @@ Function& Function::operator=(const Function& rhs)
     t_ = rhs.t_;
     rate_ = rhs.rate_;
     independent_ = rhs.independent_;
-    xs_ = rhs.xs_;
-    ys_ = rhs.ys_;
-    parser_.LinkVariables(xs_, ys_, &t_);
+
+    // Deep copy; create new Variable and constant to link with new parser.
+    // Zombification requires it. DO NOT just copy the object/pointer of
+    // MooseParser.
+    xs_.clear();
+    ys_.clear();
+    parser_.ClearAll();
+    if(rhs.parser_.GetExpr().size() > 0)
+    {
+        for(auto x: rhs.xs_)
+            xs_.push_back(shared_ptr<Variable>(new Variable()));
+        for(auto y: rhs.ys_)
+            ys_.push_back(shared_ptr<double>(new double(0.0)));
+
+        parser_.LinkVariables(xs_, ys_, &t_);
+        parser_.SetExpr(rhs.parser_.GetExpr());
+    }
     return *this;
 }
 
@@ -395,7 +393,7 @@ void Function::addVariable(const string& name)
         {
             // Equality with index because we cound from 0.
             for (size_t i = xs_.size(); i <= (size_t) index; i++)
-                xs_.push_back(new Variable());
+                xs_.push_back(shared_ptr<Variable>(new Variable()));
         }
 
         // This must be true.
@@ -414,9 +412,9 @@ void Function::addVariable(const string& name)
         {
             // Equality with index because we cound from 0.
             for (size_t i = ys_.size(); i <= (size_t) index; i++)
-                ys_.push_back(new double(0.0));
+                ys_.push_back(shared_ptr<double>(new double(0.0)));
         }
-        parser_.DefineVar(name, ys_[index]);
+        parser_.DefineVar(name, ys_[index].get());
     }
     else if (name == "t")
         parser_.DefineVar("t", &t_);
@@ -601,7 +599,7 @@ Variable * Function::getVar(unsigned int ii)
 {
     static Variable dummy;
     if ( ii < xs_.size())
-        return xs_[ii];
+        return xs_[ii].get();
 
     MOOSE_WARN( "Warning: Function::getVar: index: "
                 << ii << " is out of range: "

builtins/Function.h

@@ -24,7 +24,6 @@ class Function
 public:
     static const int VARMAX;
     Function();
-    Function(const Function& f);
 
     // Destructor.
     ~Function();
@@ -106,17 +105,15 @@ class Function
 
     // this stores variables received via incoming messages, identifiers of
     // the form x{i} are included in this
-    vector<Variable*> xs_;
+    vector<shared_ptr<Variable>> xs_;
 
     // this stores variable values pulled by sending request. identifiers of
     // the form y{i} are included in this
-    vector<double*> ys_;
+    vector<shared_ptr<double>> ys_;
 
     // Used by kinetic solvers when this is zombified.
     void* stoich_;
 
-    // Parser which should never be copied. Multithreaded programs may behave
-    // strangely if copy-constructor or operator()= is implemented.
     moose::MooseParser parser_;
 
 };

builtins/MooseParser.cpp

@@ -23,9 +23,7 @@ using namespace std;
 namespace moose
 {
 
-MooseParser::MooseParser() : 
-    expression_(new  moose::Parser::expression_t())
-    , symbol_tables_registered_(false)
+MooseParser::MooseParser()
 {
     Parser::symbol_table_t symbol_table;
 
@@ -39,8 +37,7 @@ MooseParser::MooseParser() :
     symbol_table.add_function( "srand2", MooseParser::SRand2 );
     symbol_table.add_function( "fmod", MooseParser::Fmod );
 
-    expression_->register_symbol_table(symbol_table);
-
+    expression_.register_symbol_table(symbol_table);
 }
 
 MooseParser::~MooseParser()
@@ -88,9 +85,14 @@ double MooseParser::Fmod( double a, double b )
 /*-----------------------------------------------------------------------------
  *  Get/Set
  *-----------------------------------------------------------------------------*/
-Parser::symbol_table_t& MooseParser::GetSymbolTable( ) const
+Parser::symbol_table_t& MooseParser::GetSymbolTable()
 {
-    return expression_->get_symbol_table();
+    return expression_.get_symbol_table();
+}
+
+const Parser::symbol_table_t& MooseParser::GetSymbolTable() const
+{
+    return expression_.get_symbol_table();
 }
 
 double MooseParser::GetVarValue(const string& name) const
@@ -210,7 +212,7 @@ bool MooseParser::CompileExpr()
     ASSERT_FALSE(expr_.empty(), __func__ << ": Empty expression not allowed here");
 
     Parser::parser_t  parser;
-    auto res = parser.compile(expr_, *expression_);
+    auto res = parser.compile(expr_, expression_);
     if(! res)
     {
         std::stringstream ss;
@@ -239,14 +241,14 @@ bool MooseParser::CompileExpr()
 
 double MooseParser::Derivative(const string& name) const
 {
-    return exprtk::derivative(*expression_, name);
+    return exprtk::derivative(expression_, name);
 }
 
 double MooseParser::Eval(bool check) const
 {
     if( expr_.empty())
         return 0.0;
-    double v = expression_->value();
+    double v = expression_.value();
     if(check)
     {
         if(! std::isfinite(v)) 
@@ -278,14 +280,19 @@ Parser::varmap_type MooseParser::GetConst( ) const
 
 void MooseParser::ClearVariables( )
 {
-    GetSymbolTable().clear_variables(false);
+    GetSymbolTable().clear_variables();
 }
 
 void MooseParser::ClearAll( )
 {
     ClearVariables();
 }
 
+void MooseParser::Reset( )
+{
+    expression_.release();
+}
+
 const string MooseParser::GetExpr( ) const
 {
     return expr_;
@@ -302,5 +309,17 @@ void MooseParser::LinkVariables(vector<Variable*>& xs, vector<double*>& ys, doub
     DefineVar("t", t);
 }
 
+void MooseParser::LinkVariables(vector<shared_ptr<Variable>>& xs, vector<shared_ptr<double>>& ys, double* t)
+{
+    for(size_t i = 0; i < xs.size(); i++)
+        DefineVar('x'+to_string(i), xs[i]->ref());
+
+    for (size_t i = 0; i < ys.size(); i++) 
+        DefineVar('y'+to_string(i), ys[i].get());
+
+    DefineVar("t", t);
+}
+
+
 
 } // namespace moose.