Merge branch 'master' into jelic/diff_indexed

.github/workflows/nmodl-ci.yml

@@ -32,10 +32,10 @@ jobs:
               flag_warnings: ON
               glibc_asserts: ON
               os: ubuntu-22.04
-          - config:
-              os: macos-12
           - config:
               os: macos-13
+          - config:
+              os: macos-14
           - config:
               os: windows-latest
           # TODO: might be interesting to add the thread sanitizer too
@@ -72,7 +72,7 @@ jobs:
           brew install python@3.12 || true
           brew link --overwrite python@3.12
           brew install ccache coreutils bison boost flex ninja
-          echo /usr/local/opt/flex/bin:/usr/local/opt/bison/bin >> $GITHUB_PATH
+          echo /usr/local/opt/flex/bin:/usr/local/opt/bison/bin:/opt/homebrew/opt/flex/bin:/opt/homebrew/opt/bison/bin >> $GITHUB_PATH
           # Taken from https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources
           echo CMAKE_BUILD_PARALLEL_LEVEL=3 >> $GITHUB_ENV
         shell: bash
@@ -188,7 +188,7 @@ jobs:
       - name: Build Windows
         if: startsWith(matrix.config.os, 'windows')
         run: |
-          cmake --build build
+          cmake --build build --verbose
   
       - name: Build Linux/MacOS
         if: ${{ !startsWith(matrix.config.os, 'windows') }}
@@ -209,7 +209,7 @@ jobs:
           ccache -z
           # Older versions don't support -v (verbose)
           ccache -vs 2>/dev/null || ccache -s
-          cmake --build build
+          cmake --build build --verbose
           ccache -vs 2>/dev/null || ccache -s
       - name: Test
         shell: bash

.github/workflows/sonarsource.yml

@@ -47,7 +47,7 @@ jobs:
       - name: Run build-wrapper
         working-directory: ${{runner.workspace}}/nmodl
         run: |
-          build-wrapper-linux-x86-64 --out-dir ${{ env.BUILD_WRAPPER_OUT_DIR }} cmake --build build/
+          build-wrapper-linux-x86-64 --out-dir ${{ env.BUILD_WRAPPER_OUT_DIR }} cmake --build build --verbose --parallel
       - name: Run sonar-scanner
         continue-on-error: true
         env:

CMakeLists.txt

@@ -112,7 +112,7 @@ if(NOT TARGET CLI11::CLI11)
   cpp_cc_git_submodule(cli11 BUILD PACKAGE CLI11 REQUIRED)
 endif()
 # We could have fmt incoming from NEURON
-if(NOT TARGET fmt)
+if(NOT TARGET fmt::fmt)
   cpp_cc_git_submodule(fmt BUILD EXCLUDE_FROM_ALL PACKAGE fmt REQUIRED)
 endif()
 # If we're building from the submodule, make sure we pass -fPIC so that we can link the code into a
@@ -139,11 +139,11 @@ endif()
 # installation for consistency. This line should be harmless if we use an external spdlog.
 option(SPDLOG_FMT_EXTERNAL "Force to use an external {{fmt}}" ON)
 option(SPDLOG_SYSTEM_INCLUDE "Include spdlog as a system lib" ON)
+cpp_cc_git_submodule(spdlog BUILD PACKAGE spdlog REQUIRED)
 if(NMODL_3RDPARTY_USE_SPDLOG)
   # See above, same logic as fmt
-  option(SPDLOG_BUILD_PIC "Needed to be PIC to be put compiled as a lib" ON)
+  set_target_properties(spdlog PROPERTIES POSITION_INDEPENDENT_CODE ON)
 endif()
-cpp_cc_git_submodule(spdlog BUILD PACKAGE spdlog REQUIRED)
 
 if(NMODL_ENABLE_BACKWARD)
   cpp_cc_git_submodule(backward BUILD EXCLUDE_FROM_ALL PACKAGE backward REQUIRED)

python/nmodl/ode.py

@@ -573,7 +573,13 @@ def forwards_euler2c(diff_string, dt_var, vars, function_calls):
     return f"{sp.ccode(x)} = {sp.ccode(solution, user_functions=custom_fcts)}"
 
 
-def differentiate2c(expression, dependent_var, vars, prev_expressions=None):
+def differentiate2c(
+    expression,
+    dependent_var,
+    vars,
+    prev_expressions=None,
+    stepsize=1e-3,
+):
     """Analytically differentiate supplied expression, return solution as C code.
 
     Expression should be of the form "f(x)", where "x" is
@@ -600,11 +606,15 @@ def differentiate2c(expression, dependent_var, vars, prev_expressions=None):
         vars: set of all other variables used in expression, e.g. {"a", "b", "c"}
         prev_expressions: time-ordered list of preceeding expressions
                           to evaluate & substitute, e.g. ["b = x + c", "a = 12*b"]
+        stepsize: in case an analytic expression is not possible, finite differences are used;
+                  this argument sets the step size
 
     Returns:
         string containing analytic derivative of expression (including any substitutions
         of variables from supplied prev_expressions) w.r.t. dependent_var as C code.
     """
+    if stepsize <= 0:
+        raise ValueError("arg `stepsize` must be > 0")
     prev_expressions = prev_expressions or []
     # every symbol (a.k.a variable) that SymPy
     # is going to manipulate needs to be declared
@@ -648,15 +658,27 @@ def differentiate2c(expression, dependent_var, vars, prev_expressions=None):
     # differentiate w.r.t. x
     diff = expr.diff(x).simplify()
 
+    # could be something generic like f'(x), in which case we use finite differences
+    if needs_finite_differences(diff):
+        diff = (
+            transform_expression(diff, discretize_derivative)
+            .subs({finite_difference_step_variable(x): stepsize})
+            .evalf()
+        )
+
+    # the codegen method does not like undefined function calls, so we extract
+    # them here
+    custom_fcts = {str(f.func): str(f.func) for f in diff.atoms(sp.Function)}
+
     # try to simplify expression in terms of existing variables
     # ignore any exceptions here, since we already have a valid solution
     # so if this further simplification step fails the error is not fatal
     try:
         # if expression is equal to one of the supplied vars, replace with this var
         # can do a simple string comparison here since a var cannot be further simplified
-        diff_as_string = sp.ccode(diff)
+        diff_as_string = sp.ccode(diff, user_functions=custom_fcts)
         for v in sympy_vars:
-            if diff_as_string == sp.ccode(sympy_vars[v]):
+            if diff_as_string == sp.ccode(sympy_vars[v], user_functions=custom_fcts):
                 diff = sympy_vars[v]
 
         # or if equal to rhs of one of the supplied equations, replace with lhs
@@ -677,4 +699,4 @@ def differentiate2c(expression, dependent_var, vars, prev_expressions=None):
         pass
 
     # return result as C code in NEURON format
-    return sp.ccode(diff.evalf())
+    return sp.ccode(diff.evalf(), user_functions=custom_fcts)

src/codegen/codegen_cpp_visitor.cpp

@@ -198,12 +198,12 @@ int CodegenCppVisitor::int_variables_size() const {
  * representation (1e+20, 1E-15) then keep it as it is.
  */
 std::string CodegenCppVisitor::format_double_string(const std::string& s_value) {
-    return utils::format_double_string<CodegenCppVisitor>(s_value);
+    return utils::format_double_string(s_value);
 }
 
 
 std::string CodegenCppVisitor::format_float_string(const std::string& s_value) {
-    return utils::format_float_string<CodegenCppVisitor>(s_value);
+    return utils::format_float_string(s_value);
 }
 
 
@@ -1728,7 +1728,6 @@ void CodegenCppVisitor::print_rename_state_vars() const {
         auto rhs = get_variable_name(state_name + "0");
 
         if (state->is_array()) {
-            auto size = state->get_length();
             for (int i = 0; i < state->get_length(); ++i) {
                 printer->fmt_line("{}[{}] = {};", lhs, i, rhs);
             }

src/codegen/codegen_utils.cpp

@@ -20,8 +20,7 @@ namespace utils {
  * they are represented in the mod file by user. If the value is in scientific
  * representation (1e+20, 1E-15) then keep it as it is.
  */
-template <>
-std::string format_double_string<CodegenCppVisitor>(const std::string& s_value) {
+std::string format_double_string(const std::string& s_value) {
     double value = std::stod(s_value);
     if (std::ceil(value) == value && s_value.find_first_of("eE") == std::string::npos) {
         return fmt::format("{:.1f}", value);
@@ -30,8 +29,7 @@ std::string format_double_string<CodegenCppVisitor>(const std::string& s_value)
 }
 
 
-template <>
-std::string format_float_string<CodegenCppVisitor>(const std::string& s_value) {
+std::string format_float_string(const std::string& s_value) {
     float value = std::stof(s_value);
     if (std::ceil(value) == value && s_value.find_first_of("eE") == std::string::npos) {
         return fmt::format("{:.1f}", value);

src/codegen/codegen_utils.hpp

@@ -29,7 +29,6 @@ namespace utils {
  * \param s_value The double constant as string
  * \return        The proper string to be printed in the generated file.
  */
-template <typename T>
 std::string format_double_string(const std::string& s_value);
 
 
@@ -43,7 +42,6 @@ std::string format_double_string(const std::string& s_value);
  * \param s_value The double constant as string
  * \return        The proper string to be printed in the generated file.
  */
-template <typename T>
 std::string format_float_string(const std::string& s_value);
 
 }  // namespace utils

src/main.cpp

@@ -314,8 +314,10 @@ int run_nmodl(int argc, const char* argv[]) {
         /// create file path for nmodl file
         auto filepath = [scratch_dir, modfile](const std::string& suffix) {
             static int count = 0;
-            return fmt::format(
-                "{}/{}.{}.{}.mod", scratch_dir, modfile, std::to_string(count++), suffix);
+
+            auto filename = fmt::format("{}.{}.{}.mod", modfile, std::to_string(count++), suffix);
+
+            return (std::filesystem::path(scratch_dir) / filename).string();
         };
 
         /// driver object creates lexer and parser, just call parser method
@@ -328,9 +330,6 @@ int run_nmodl(int argc, const char* argv[]) {
         /// one whenever we run symtab visitor.
         bool update_symtab = false;
 
-        /// just visit the ast
-        AstVisitor().visit_program(*ast);
-
         {
             logger->info("Running argument renaming visitor");
             RenameFunctionArgumentsVisitor().visit_program(*ast);
@@ -406,12 +405,10 @@ int run_nmodl(int argc, const char* argv[]) {
         ast_to_nmodl(*ast, filepath("ast"));
 
         if (json_ast) {
-            std::string file{scratch_dir};
-            file += "/";
-            file += modfile;
-            file += ".ast.json";
-            logger->info("Writing AST into {}", file);
-            JSONVisitor(file).write(*ast);
+            std::filesystem::path file{scratch_dir};
+            file /= modfile + ".ast.json";
+            logger->info("Writing AST into {}", file.string());
+            JSONVisitor(file.string()).write(*ast);
         }
 
         if (verbatim_rename) {

src/utils/CMakeLists.txt

@@ -13,7 +13,7 @@ add_library(
   ${PROJECT_BINARY_DIR}/src/config/config.cpp)
 
 set_property(TARGET util PROPERTY POSITION_INDEPENDENT_CODE ON)
-target_link_libraries(util PUBLIC fmt::fmt nlohmann_json::nlohmann_json spdlog::spdlog_header_only)
+target_link_libraries(util PUBLIC fmt::fmt nlohmann_json::nlohmann_json spdlog::spdlog)
 
 if(NMODL_ENABLE_BACKWARD)
   target_link_libraries(util PRIVATE Backward::Interface)

test/unit/CMakeLists.txt

@@ -17,7 +17,7 @@ include_directories(${NMODL_PROJECT_SOURCE_DIR}/ext/eigen)
 # Common input data library
 # =============================================================================
 add_library(test_util STATIC utils/nmodl_constructs.cpp utils/test_utils.cpp)
-target_link_libraries(test_util PUBLIC spdlog::spdlog_header_only)
+target_link_libraries(test_util PUBLIC spdlog::spdlog)
 
 # =============================================================================
 # Common input data library

test/unit/codegen/codegen_utils.cpp

@@ -21,8 +21,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
         std::string double_constant = "0.012345678901234567";
 
         THEN("Codegen C Visitor prints double with same precision") {
-            auto nmodl_constant_result = codegen::utils::format_double_string<CodegenCppVisitor>(
-                double_constant);
+            auto nmodl_constant_result = codegen::utils::format_double_string(double_constant);
             REQUIRE(nmodl_constant_result == double_constant);
         }
     }
@@ -33,8 +32,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
         std::string codegen_output = "1.0";
 
         THEN("Codegen C Visitor prints integer as double number") {
-            auto nmodl_constant_result = codegen::utils::format_double_string<CodegenCppVisitor>(
-                double_constant);
+            auto nmodl_constant_result = codegen::utils::format_double_string(double_constant);
             REQUIRE(nmodl_constant_result == codegen_output);
         }
     }
@@ -44,8 +42,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
 
         THEN("Codegen C Visitor prints doubles with scientific notation") {
             for (const auto& test: tests) {
-                REQUIRE(codegen::utils::format_double_string<CodegenCppVisitor>(test.first) ==
-                        test.second);
+                REQUIRE(codegen::utils::format_double_string(test.first) == test.second);
             }
         }
     }
@@ -54,8 +51,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
         std::string float_constant = "0.01234567";
 
         THEN("Codegen C Visitor prints float with same precision") {
-            auto nmodl_constant_result = codegen::utils::format_float_string<CodegenCppVisitor>(
-                float_constant);
+            auto nmodl_constant_result = codegen::utils::format_float_string(float_constant);
             REQUIRE(nmodl_constant_result == float_constant);
         }
     }
@@ -66,8 +62,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
         std::string codegen_output = "1.0";
 
         THEN("Codegen C Visitor prints integer as double number") {
-            auto nmodl_constant_result = codegen::utils::format_float_string<CodegenCppVisitor>(
-                float_constant);
+            auto nmodl_constant_result = codegen::utils::format_float_string(float_constant);
             REQUIRE(nmodl_constant_result == codegen_output);
         }
     }
@@ -77,8 +72,7 @@ SCENARIO("C codegen utility functions", "[codegen][util][c]") {
 
         THEN("Codegen C Visitor prints doubles with scientific notation") {
             for (const auto& test: tests) {
-                REQUIRE(codegen::utils::format_float_string<CodegenCppVisitor>(test.first) ==
-                        test.second);
+                REQUIRE(codegen::utils::format_float_string(test.first) == test.second);
             }
         }
     }

test/unit/ode/test_ode.py

@@ -3,7 +3,12 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
+<<<<<<< HEAD
 from nmodl.ode import differentiate2c, integrate2c, make_symbol
+=======
+from nmodl.ode import differentiate2c, integrate2c
+import pytest
+>>>>>>> master
 
 import sympy as sp
 
@@ -118,6 +123,38 @@ def test_differentiate2c():
         {sp.IndexedBase("s", shape=[1]), sp.IndexedBase("z", shape=[1])},
     )
 
+    result = differentiate2c(
+        "-f(x)",
+        "x",
+        {},
+    )
+    # instead of comparing the expression as a string, we convert the string
+    # back to an expression and compare with an explicit function
+    size = 100
+    for index in range(size):
+        a, b = -5, 5
+        value = (b - a) * index / size + a
+        pytest.approx(
+            float(
+                sp.sympify(result)
+                .subs(sp.Function("f"), sp.sin)
+                .subs({"x": value})
+                .evalf()
+            )
+        ) == float(
+            -sp.Derivative(sp.sin("x"))
+            .as_finite_difference(1e-3)
+            .subs({"x": value})
+            .evalf()
+        )
+    with pytest.raises(ValueError):
+        differentiate2c(
+            "-f(x)",
+            "x",
+            {},
+            stepsize=-1,
+        )
+
 
 def test_integrate2c():
 

test/unit/units/parser.cpp

@@ -27,6 +27,7 @@ using Catch::Matchers::ContainsSubstring;
 // able to define complex units based on base units
 nmodl::parser::UnitDriver driver;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
 
+namespace {
 bool is_valid_construct(const std::string& construct) {
     return driver.parse_string(construct);
 }
@@ -40,6 +41,7 @@ std::string parse_string(const std::string& unit_definition) {
     correctness_driver.table->print_base_units(ss);
     return ss.str();
 }
+}  // namespace
 
 SCENARIO("Unit parser accepting valid units definition", "[unit][parser]") {
     GIVEN("A base unit") {