From 6be1a19b2858e7819c67c8eb12a466cff2a7e260 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Fri, 12 Jul 2024 15:54:56 +0200 Subject: [PATCH 01/18] Implement printing for CONSTANT. (#1339) * Implement printing for CONSTANT. * Document CONSTANT. * Register `constant` usecase group. --- docs/contents/globals.rst | 16 ++++++++++++++++ src/codegen/codegen_neuron_cpp_visitor.cpp | 11 +++++++++++ test/usecases/CMakeLists.txt | 1 + test/usecases/constant/constant.mod | 11 +++++++++++ test/usecases/constant/test_constant.py | 15 +++++++++++++++ 5 files changed, 54 insertions(+) create mode 100644 test/usecases/constant/constant.mod create mode 100644 test/usecases/constant/test_constant.py diff --git a/docs/contents/globals.rst b/docs/contents/globals.rst index 418b478df..ce3fa3247 100644 --- a/docs/contents/globals.rst +++ b/docs/contents/globals.rst @@ -5,6 +5,7 @@ The following global variables exist: * ``GLOBAL`` visible from HOC/Python. * ``LOCAL`` at file scope, called top-locals. + * ``CONSTANT`` not visible from HOC/Python. GLOBAL variables ================ @@ -175,6 +176,21 @@ Collection of slightly surprising behaviour: the code ``nocmodl`` produces will cause a SEGFAULT. +CONSTANT variables +================== + +These are comparatively simple. In NOCMODL they're implemented as non-const +static doubles. They're not accessible from HOC/Python (which makes them +simple). + +Quirks +~~~~~~ + +In certain versions of NOCMODL around `9.0` and before (and NMODL) it's +possible to change the value of CONSTANT variables. The MOD file will still be +considered "thread-safe" (even if it might not be). + + What Does CoreNEURON support? ============================= CoreNEURON only supports read-only GLOBAL variables. Anything else needs to be diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index d3688e984..e6db2c953 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -980,6 +980,17 @@ void CodegenNeuronCppVisitor::print_mechanism_global_var_structure(bool print_in codegen_global_variables.push_back(var); } + for (const auto& var: info.constant_variables) { + auto const name = var->get_name(); + auto* const value_ptr = var->get_value().get(); + double const value{value_ptr ? *value_ptr : 0}; + printer->fmt_line("{} {}{};", + float_type, + name, + print_initializers ? fmt::format("{{{:g}}}", value) : std::string{}); + codegen_global_variables.push_back(var); + } + // for (const auto& f: info.function_tables) { if (!info.function_tables.empty()) { diff --git a/test/usecases/CMakeLists.txt b/test/usecases/CMakeLists.txt index 41a13dcb1..3923727b4 100644 --- a/test/usecases/CMakeLists.txt +++ b/test/usecases/CMakeLists.txt @@ -1,5 +1,6 @@ set(NMODL_USECASE_DIRS cnexp + constant function procedure global diff --git a/test/usecases/constant/constant.mod b/test/usecases/constant/constant.mod new file mode 100644 index 000000000..f1b6cc854 --- /dev/null +++ b/test/usecases/constant/constant.mod @@ -0,0 +1,11 @@ +NEURON { + SUFFIX constant_mod +} + +CONSTANT { + a = 2.3 +} + +FUNCTION foo() { + foo = a +} diff --git a/test/usecases/constant/test_constant.py b/test/usecases/constant/test_constant.py new file mode 100644 index 000000000..857566f08 --- /dev/null +++ b/test/usecases/constant/test_constant.py @@ -0,0 +1,15 @@ +import numpy as np + +from neuron import h, gui +from neuron.units import ms + +nseg = 1 +s = h.Section() +s.insert("constant_mod") + +expected = 2.3 +assert s(0.5).constant_mod.foo() == expected + +h.stdinit() + +assert s(0.5).constant_mod.foo() == expected From 2af37abb8ca3a5c62718cacc8a853aa448cac462 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Fri, 12 Jul 2024 15:55:16 +0200 Subject: [PATCH 02/18] Don't print `int*` range variables. (#1340) * Add regression test. * Don't print `int*` range variables. --- src/codegen/codegen_neuron_cpp_visitor.cpp | 3 +-- test/usecases/useion/style_ion.mod | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 test/usecases/useion/style_ion.mod diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index e6db2c953..42e2f5d84 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -1354,8 +1354,7 @@ void CodegenNeuronCppVisitor::print_mechanism_range_var_structure(bool print_ini if (name == naming::POINT_PROCESS_VARIABLE) { continue; } else if (var.is_index || var.is_integer) { - auto qualifier = var.is_constant ? "const " : ""; - printer->fmt_line("{}{}* const* {}{};", qualifier, int_type, name, value_initialize); + // In NEURON we don't create caches for `int*`. Hence, do nothing. } else { auto qualifier = var.is_constant ? "const " : ""; auto type = var.is_vdata ? "void*" : default_float_data_type(); diff --git a/test/usecases/useion/style_ion.mod b/test/usecases/useion/style_ion.mod new file mode 100644 index 000000000..4e6ba180f --- /dev/null +++ b/test/usecases/useion/style_ion.mod @@ -0,0 +1,18 @@ +: This checks for a code-generation bug that resulted in a faulty ctor +: call. It requires an ion to have a "style", followed by other ions. + +NEURON { + SUFFIX style_ion + USEION ca WRITE cai, eca + USEION na READ nai +} + +ASSIGNED { + cai + eca + nai +} + +INITIAL { + cai = 42.0 +} From 9a430a0d1ced88ebfcdbe1a7cf5dd08964f2011d Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 15 Jul 2024 10:27:57 +0200 Subject: [PATCH 03/18] Implement VALENCE. (#1341) --- src/codegen/codegen_helper_visitor.cpp | 12 +++++++++++- src/codegen/codegen_info.hpp | 4 ++++ src/codegen/codegen_neuron_cpp_visitor.cpp | 4 +++- test/usecases/useion/test_valence.py | 9 +++++++++ test/usecases/useion/valence.mod | 14 ++++++++++++++ 5 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 test/usecases/useion/test_valence.py create mode 100644 test/usecases/useion/valence.mod diff --git a/src/codegen/codegen_helper_visitor.cpp b/src/codegen/codegen_helper_visitor.cpp index 417eca131..5a2880b77 100644 --- a/src/codegen/codegen_helper_visitor.cpp +++ b/src/codegen/codegen_helper_visitor.cpp @@ -73,16 +73,22 @@ void CodegenHelperVisitor::find_ion_variables(const ast::Program& node) { std::vector ion_vars; std::vector read_ion_vars; std::vector write_ion_vars; + std::map valences; for (const auto& ion_node: ion_nodes) { const auto& ion = std::dynamic_pointer_cast(ion_node); - ion_vars.push_back(ion->get_node_name()); + auto ion_name = ion->get_node_name(); + ion_vars.push_back(ion_name); for (const auto& var: ion->get_readlist()) { read_ion_vars.push_back(var->get_node_name()); } for (const auto& var: ion->get_writelist()) { write_ion_vars.push_back(var->get_node_name()); } + + if (ion->get_valence() != nullptr) { + valences[ion_name] = ion->get_valence()->get_value()->to_double(); + } } /** @@ -112,6 +118,10 @@ void CodegenHelperVisitor::find_ion_variables(const ast::Program& node) { } } } + if (auto it = valences.find(ion_name); it != valences.end()) { + ion.valence = it->second; + } + info.ions.push_back(std::move(ion)); } diff --git a/src/codegen/codegen_info.hpp b/src/codegen/codegen_info.hpp index c3274ba59..4f392cc68 100644 --- a/src/codegen/codegen_info.hpp +++ b/src/codegen/codegen_info.hpp @@ -13,6 +13,7 @@ */ #include +#include #include #include #include @@ -61,6 +62,9 @@ struct Ion { /// ion variables that are being written std::vector writes; + /// ion valence + std::optional valence; + /// if style semantic needed bool need_style = false; diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index 42e2f5d84..cacaf6dcc 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -1138,7 +1139,8 @@ void CodegenNeuronCppVisitor::print_mechanism_register() { printer->add_newline(); for (const auto& ion: info.ions) { - printer->fmt_line("ion_reg(\"{}\", {});", ion.name, "-10000."); + double valence = ion.valence.value_or(-10000.0); + printer->fmt_line("ion_reg(\"{}\", {});", ion.name, valence); } if (!info.ions.empty()) { printer->add_newline(); diff --git a/test/usecases/useion/test_valence.py b/test/usecases/useion/test_valence.py new file mode 100644 index 000000000..d24611f98 --- /dev/null +++ b/test/usecases/useion/test_valence.py @@ -0,0 +1,9 @@ +from neuron import h + + +def test_valence(): + assert h.ion_charge("K_ion") == 222.0 + + +if __name__ == "__main__": + test_valence() diff --git a/test/usecases/useion/valence.mod b/test/usecases/useion/valence.mod new file mode 100644 index 000000000..6ca8b857f --- /dev/null +++ b/test/usecases/useion/valence.mod @@ -0,0 +1,14 @@ +NEURON { + SUFFIX valence_mod + USEION K READ Ki VALENCE 222 + RANGE x +} + +ASSIGNED { + x + Ki +} + +INITIAL { + x = Ki +} From 2387ba72e61400d22cea66cc663f59134a5969a8 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 15 Jul 2024 10:28:13 +0200 Subject: [PATCH 04/18] Fix bug in TABLE for POINT_PROCESS. (#1342) * Regression test. * Improve update_table name. Change the name to `update_table_*`. To avoid inconsistencies in the name (and it's suffix) we introduce a function `table_update_function_name`. Fixes a bug related to incorrect use of `rsuffix`, which caused the function name to be inconsistent for POINT_PROCESSES. --- .../codegen_coreneuron_cpp_visitor.cpp | 2 +- src/codegen/codegen_cpp_visitor.cpp | 9 ++-- src/codegen/codegen_cpp_visitor.hpp | 8 +-- src/codegen/codegen_neuron_cpp_visitor.cpp | 17 +++--- test/usecases/table/simulate.py | 53 +++++++++++++------ test/usecases/table/table_point_process.mod | 44 +++++++++++++++ 6 files changed, 97 insertions(+), 36 deletions(-) create mode 100644 test/usecases/table/table_point_process.mod diff --git a/src/codegen/codegen_coreneuron_cpp_visitor.cpp b/src/codegen/codegen_coreneuron_cpp_visitor.cpp index 2735d3000..74581bbaa 100644 --- a/src/codegen/codegen_coreneuron_cpp_visitor.cpp +++ b/src/codegen/codegen_coreneuron_cpp_visitor.cpp @@ -418,7 +418,7 @@ void CodegenCoreneuronCppVisitor::print_check_table_thread_function() { printer->add_line("double v = 0;"); for (const auto& function: info.functions_with_table) { - auto method_name_str = method_name(table_function_prefix() + function->get_node_name()); + auto method_name_str = table_update_function_name(function->get_node_name()); auto arguments = internal_method_arguments(); printer->fmt_line("{}({});", method_name_str, arguments); } diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp index e87e6aa53..611ff26b4 100644 --- a/src/codegen/codegen_cpp_visitor.cpp +++ b/src/codegen/codegen_cpp_visitor.cpp @@ -72,8 +72,8 @@ bool CodegenCppVisitor::has_parameter_of_name(const T& node, const std::string& } -std::string CodegenCppVisitor::table_function_prefix() const { - return "lazy_update_"; +std::string CodegenCppVisitor::table_update_function_name(const std::string& block_name) const { + return "update_table_" + method_name(block_name); } @@ -1524,9 +1524,8 @@ void CodegenCppVisitor::print_table_check_function(const Block& node) { auto float_type = default_float_data_type(); printer->add_newline(2); - printer->fmt_push_block("void {}{}({})", - table_function_prefix(), - method_name(name), + printer->fmt_push_block("void {}({})", + table_update_function_name(name), get_parameter_str(internal_params)); { printer->fmt_push_block("if ({} == 0)", use_table_var); diff --git a/src/codegen/codegen_cpp_visitor.hpp b/src/codegen/codegen_cpp_visitor.hpp index 875587acd..8a1415c23 100644 --- a/src/codegen/codegen_cpp_visitor.hpp +++ b/src/codegen/codegen_cpp_visitor.hpp @@ -1112,10 +1112,12 @@ class CodegenCppVisitor: public visitor::ConstAstVisitor { bool use_instance = true) const = 0; /** - * Prefix used for the function that performs the lazy update + * The name of the function that updates the table value if the parameters + * changed. + * + * \param block_name The name of the block that contains the TABLE. */ - std::string table_function_prefix() const; - + std::string table_update_function_name(const std::string& block_name) const; /** * Return ion variable name and corresponding ion read variable name. diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index cacaf6dcc..9b8ddbba2 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -165,9 +165,8 @@ void CodegenNeuronCppVisitor::print_check_table_function_prototypes() { for (const auto& function: info.functions_with_table) { auto name = function->get_node_name(); auto internal_params = internal_method_parameters(); - printer->fmt_line("void {}{}({});", - table_function_prefix(), - method_name(name), + printer->fmt_line("void {}({});", + table_update_function_name(name), get_parameter_str(internal_params)); } @@ -193,10 +192,9 @@ void CodegenNeuronCppVisitor::print_check_table_function_prototypes() { } for (const auto& function: info.functions_with_table) { - auto method_name_str = function->get_node_name(); - auto method_args_str = get_arg_str(internal_method_parameters()); - printer->fmt_line( - "{}{}{}({});", table_function_prefix(), method_name_str, info.rsuffix, method_args_str); + auto method_name = function->get_node_name(); + auto method_args = get_arg_str(internal_method_parameters()); + printer->fmt_line("{}({});", table_update_function_name(method_name), method_args); } printer->pop_block(); } @@ -404,9 +402,8 @@ void CodegenNeuronCppVisitor::print_hoc_py_wrapper_function_body( info.thread_var_thread_id); } if (info.function_uses_table(block_name)) { - printer->fmt_line("{}{}({});", - table_function_prefix(), - method_name(block_name), + printer->fmt_line("{}({});", + table_update_function_name(block_name), internal_method_arguments()); } const auto get_func_call_str = [&]() { diff --git a/test/usecases/table/simulate.py b/test/usecases/table/simulate.py index a4aa5e397..190e4ff28 100644 --- a/test/usecases/table/simulate.py +++ b/test/usecases/table/simulate.py @@ -14,14 +14,14 @@ def check_solution(y_no_table, y_table, rtol): ), f"{y_no_table} == {y_table}" -def check_table(c1, c2, x, evaluate_table): - h.c1_tbl = 1 - h.c2_tbl = 2 +def check_table(c1, c2, x, mech_name, evaluate_table): + setattr(h, f"c1_{mech_name}", 1) + setattr(h, f"c2_{mech_name}", 2) - h.usetable_tbl = 0 + setattr(h, f"usetable_{mech_name}", 0) y_no_table = np.array([evaluate_table(i) for i in x]) - h.usetable_tbl = 1 + setattr(h, f"usetable_{mech_name}", 1) y_table = np.array([evaluate_table(i) for i in x]) check_solution(y_table, y_no_table, rtol=1e-4) @@ -31,34 +31,52 @@ def check_table(c1, c2, x, evaluate_table): assert np.all(evaluate_table(x[-1] + 10) == y_table[-1]) -def test_function(): +def check_function(mech_name, make_instance): s = h.Section() - s.insert("tbl") + obj = make_instance(s) x = np.linspace(-3, 5, 18) assert x[0] == -3.0 assert x[-1] == 5.0 - check_table(1, 2, x, s(0.5).tbl.quadratic) - check_table(2, 2, x, s(0.5).tbl.quadratic) - check_table(2, 3, x, s(0.5).tbl.quadratic) + check_table(1, 2, x, mech_name, obj.quadratic) + check_table(2, 2, x, mech_name, obj.quadratic) + check_table(2, 3, x, mech_name, obj.quadratic) -def test_procedure(): - s = h.Section() +def make_density_instance(s): s.insert("tbl") + return s(0.5).tbl + + +def make_point_instance(s): + return h.tbl_point_process(s(0.5)) + + +def test_function(): + check_function("tbl", make_density_instance) + check_function("tbl_point_process", make_point_instance) + + +def check_procedure(mech_name, make_instance): + s = h.Section() + obj = make_instance(s) def evaluate_table(x): - s(0.5).tbl.sinusoidal(x) - return np.array((s(0.5).tbl.v1, s(0.5).tbl.v2)) + obj.sinusoidal(x) + return np.array((obj.v1, obj.v2)) x = np.linspace(-4, 6, 18) assert x[0] == -4.0 assert x[-1] == 6.0 - check_table(1, 2, x, evaluate_table) - check_table(2, 2, x, evaluate_table) - check_table(2, 3, x, evaluate_table) + check_table(1, 2, x, mech_name, evaluate_table) + check_table(2, 2, x, mech_name, evaluate_table) + check_table(2, 3, x, mech_name, evaluate_table) + + +def test_procedure(): + check_procedure("tbl", make_density_instance) def simulate(): @@ -83,4 +101,5 @@ def simulate(): if __name__ == "__main__": test_function() test_procedure() + simulate() diff --git a/test/usecases/table/table_point_process.mod b/test/usecases/table/table_point_process.mod new file mode 100644 index 000000000..50da1aeb1 --- /dev/null +++ b/test/usecases/table/table_point_process.mod @@ -0,0 +1,44 @@ +NEURON { + POINT_PROCESS tbl_point_process + NONSPECIFIC_CURRENT i + RANGE g, v1, v2 + GLOBAL k, d, c1, c2 +} + +PARAMETER { + k = .1 + d = -50 + c1 = 1 + c2 = 2 +} + +ASSIGNED { + g + i + v + sig + v1 + v2 +} + +BREAKPOINT { + sigmoidal(v) + g = 0.001 * sig + i = g*(v - 30.0) +} + +PROCEDURE sigmoidal(v) { + TABLE sig DEPEND k, d FROM -127 TO 128 WITH 155 + sig = 1/(1 + exp(k*(v - d))) +} + +FUNCTION quadratic(x) { + TABLE DEPEND c1, c2 FROM -3 TO 5 WITH 500 + quadratic = c1 * x * x + c2 +} + +PROCEDURE sinusoidal(x) { + TABLE v1, v2 DEPEND c1, c2 FROM -4 TO 6 WITH 800 + v1 = sin(c1 * x) + 2 + v2 = cos(c2 * x) + 2 +} From 094a6abfb42e711e3b5b1ba578fca9727e07e8c4 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 15 Jul 2024 10:28:27 +0200 Subject: [PATCH 05/18] Fix FUNCTION, PROCEDURE with Newton. (#1343) --- src/codegen/codegen_neuron_cpp_visitor.cpp | 9 +-------- test/usecases/kinetic/X2Y.mod | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index 9b8ddbba2..f1e25affe 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -829,15 +829,8 @@ void CodegenNeuronCppVisitor::print_sdlists_init([[maybe_unused]] bool print_ini } CodegenCppVisitor::ParamVector CodegenNeuronCppVisitor::functor_params() { - auto params = ParamVector{}; - params.push_back({"", "NrnThread*", "", "nt"}); - params.push_back({"", fmt::format("{}&", instance_struct()), "", "inst"}); - params.push_back({"", "int", "", "id"}); + auto params = internal_method_parameters(); params.push_back({"", "double", "", "v"}); - params.push_back({"", "Datum*", "", "_thread"}); - if (!codegen_thread_variables.empty()) { - params.push_back({"", fmt::format("{}&", thread_variables_struct()), "", "_thread_vars"}); - } return params; } diff --git a/test/usecases/kinetic/X2Y.mod b/test/usecases/kinetic/X2Y.mod index 5ef3f853a..5d9c72a49 100644 --- a/test/usecases/kinetic/X2Y.mod +++ b/test/usecases/kinetic/X2Y.mod @@ -1,6 +1,6 @@ NEURON { SUFFIX X2Y - RANGE X, Y + RANGE X, Y, c1, c2 NONSPECIFIC_CURRENT il } @@ -12,6 +12,8 @@ STATE { ASSIGNED { il i + c1 + c2 } BREAKPOINT { @@ -22,9 +24,17 @@ BREAKPOINT { INITIAL { X = 0 Y = 1 + c1 = 0.0 + c2 = 0.0 } KINETIC state { - ~ X <-> Y (0.4, 0.5) + rates() + ~ X <-> Y (c1, c2) i = (f_flux-b_flux) } + +PROCEDURE rates() { + c1 = 0.4 + c2 = 0.5 +} From 0193a99b58ada3afa5b211190fb55ee7c754c88c Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 15 Jul 2024 17:34:53 +0200 Subject: [PATCH 06/18] Conditionally print Newton boiler plate. (#1345) --- src/codegen/codegen_neuron_cpp_visitor.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index f1e25affe..444813c20 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -768,10 +768,10 @@ void CodegenNeuronCppVisitor::print_standard_includes() { #include #include #include + #include )CODE"); - printer->add_multi_line(nmodl::solvers::newton_hpp); - if (!info.vectorize) { - printer->add_line("#include "); + if (info.eigen_newton_solver_exist) { + printer->add_multi_line(nmodl::solvers::newton_hpp); } } From dd9a52d5cb64eab4659f7d58a16fc96a4249b972 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Tue, 16 Jul 2024 16:26:11 +0200 Subject: [PATCH 07/18] Fix RANGE PARAMETER defaults. (#1346) * Add documentation of PARAMETER. * Fix RANGE PARAMETER defaults. PARAMETERs that are RANGE variables need to register their default values, because NEURON needs to be able to initialize the parameters with their default values. This commits adds the registration code, and sets defaults in `nrn_alloc` from the same array. * Add units®ression tests. --- docs/contents/globals.rst | 25 +++++++++ src/codegen/codegen_neuron_cpp_visitor.cpp | 22 +++++++- src/codegen/codegen_neuron_cpp_visitor.hpp | 3 + test/usecases/parameter/default_parameter.mod | 14 +++++ test/usecases/parameter/test_default.py | 55 +++++++++++++++++++ 5 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 test/usecases/parameter/default_parameter.mod create mode 100644 test/usecases/parameter/test_default.py diff --git a/docs/contents/globals.rst b/docs/contents/globals.rst index ce3fa3247..3825d8ba6 100644 --- a/docs/contents/globals.rst +++ b/docs/contents/globals.rst @@ -176,6 +176,31 @@ Collection of slightly surprising behaviour: the code ``nocmodl`` produces will cause a SEGFAULT. +PARAMETER variables +=================== + +These can be either RANGE or not RANGE. They can be both read and write. If +they're written to, they're converted to thread-variables. Therefore, the rest +of this section will only describe read-only PARAMETERs. + +Additionally, parameters optionally have: a) a default value, b) units and c) a +valid range. + +Default Values +~~~~~~~~~~~~~~ +This section only applies to read-only PARAMETERs. + +The behaviour differs for RANGE variables and non-RANGE variables. For RANGE +variables, default values need to be registered with NEURON for all PARAMETERs +that are RANGE variables. The function is called ``hoc_register_parm_default``. + +Note, that NOCMODL uses it in the ``nrn_alloc`` in the generated `.cpp` files +and also in ``ndatclas.cpp``. Therefore, it seems registering the values isn't +optional. + +Non-RANGE variables are semantically equivalent to ``static double``. They're +simply assigned their value in the definition of the global variable. + CONSTANT variables ================== diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index 444813c20..bea1f104e 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -997,6 +997,21 @@ void CodegenNeuronCppVisitor::print_mechanism_global_var_structure(bool print_in print_global_var_struct_assertions(); print_global_var_struct_decl(); + + print_global_param_default_values(); +} + +void CodegenNeuronCppVisitor::print_global_param_default_values() { + printer->push_block("static std::vector _parameter_defaults ="); + + std::vector defaults; + for (const auto& p: info.range_parameter_vars) { + double value = p->get_value() == nullptr ? 0.0 : *p->get_value(); + defaults.push_back(fmt::format("{:g} /* {} */", value, p->get_name())); + } + + printer->add_multi_line(fmt::format("{}", fmt::join(defaults, ",\n"))); + printer->pop_block(";"); } /// TODO: Same as CoreNEURON? @@ -1170,6 +1185,8 @@ void CodegenNeuronCppVisitor::print_mechanism_register() { printer->add_newline(); printer->fmt_line("mech_type = nrn_get_mechtype({}[1]);", get_channel_info_var_name()); + printer->add_line("hoc_register_parm_default(mech_type, &_parameter_defaults);"); + // register the table-checking function if (info.table_count > 0) { printer->fmt_line("_nrn_thread_table_reg(mech_type, {});", table_thread_function_name()); @@ -1617,7 +1634,8 @@ void CodegenNeuronCppVisitor::print_nrn_alloc() { codegen_float_variables.size()); if (float_variables_size()) { printer->add_line("/*initialize range parameters*/"); - for (const auto& var: info.range_parameter_vars) { + for (size_t i_param = 0; i_param < info.range_parameter_vars.size(); ++i_param) { + const auto var = info.range_parameter_vars[i_param]; if (var->is_array()) { continue; } @@ -1627,7 +1645,7 @@ void CodegenNeuronCppVisitor::print_nrn_alloc() { printer->fmt_line("_lmc.template fpfield<{}>(_iml) = {}; /* {} */", var_pos, - var_value, + fmt::format("_parameter_defaults[{}]", i_param), var_name); } } diff --git a/src/codegen/codegen_neuron_cpp_visitor.hpp b/src/codegen/codegen_neuron_cpp_visitor.hpp index 7cac67c2f..f23aa9b1c 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.hpp +++ b/src/codegen/codegen_neuron_cpp_visitor.hpp @@ -470,6 +470,9 @@ class CodegenNeuronCppVisitor: public CodegenCppVisitor { */ void print_global_variables_for_hoc() override; + /** Print global struct with default value of RANGE PARAMETERs. + */ + void print_global_param_default_values(); /** * Print the mechanism registration function diff --git a/test/usecases/parameter/default_parameter.mod b/test/usecases/parameter/default_parameter.mod new file mode 100644 index 000000000..d31d91aa0 --- /dev/null +++ b/test/usecases/parameter/default_parameter.mod @@ -0,0 +1,14 @@ +NEURON { + SUFFIX default_parameter + RANGE x, y, z + GLOBAL a +} + +PARAMETER { + a + b = 0.1 + + x + y = 2.1 + z +} diff --git a/test/usecases/parameter/test_default.py b/test/usecases/parameter/test_default.py new file mode 100644 index 000000000..400f533d6 --- /dev/null +++ b/test/usecases/parameter/test_default.py @@ -0,0 +1,55 @@ +from neuron import h + + +def check_defaults(obj, parameters): + defaults = { + "b": 0.1, + "y": 2.1, + } + + for p in parameters: + actual = getattr(obj, f"{p}_default_parameter") + expected = defaults.get(p, 0.0) + assert actual == expected + + +def test_default_values(): + nseg = 10000 + + s = h.Section() + s.nseg = nseg + s.insert("default_parameter") + + static_parameters = ["a", "b"] + range_parameters = ["x", "y", "z"] + + check_defaults(h, static_parameters) + for seg in s: + check_defaults(seg, range_parameters) + + h.finitialize() + + check_defaults(h, static_parameters) + for seg in s: + check_defaults(seg, range_parameters) + + +def test_parcom_regression(): + # This is a roundabout way of triggering a codepath leading to + # NrnProperyImpl. In NrnProperyImpl we iterate over the std::vector of + # default values. Hence, it requires that the default values are registered + # correctly. + + nseg = 5 + + s = h.Section() + s.nseg = nseg + s.insert("default_parameter") + + # Caused a SEGFAULT: + h.load_file("parcom.hoc") + + +if __name__ == "__main__": + test_default_values() + test_parcom_regression() From 84744113efb93294f83cbd9660d52da98b75426f Mon Sep 17 00:00:00 2001 From: Pramod Kumbhar Date: Tue, 16 Jul 2024 18:03:14 +0200 Subject: [PATCH 08/18] Remove unnecessary warning messages (#1350) - Looking at the typical warnings produced by NMODL, two that I find spurious: * Defining a variable of the same name in a new scope ("shadowing"). * Unable to inline a function or procedure. - Both of these should not be warnings. The first is perfectly acceptable, and the second can occur in cases where TABLE is used. Emitting these warnings is more misleading than helpful IMO. - We are converting these two warnings to debug level messages as it might be helpful to know these situations while debugging. - An example from the Snudda model: ``` [NMODL] [warning] :: SYMTAB :: ca [Argument] in rate shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: ca [Argument] in rate shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: cai [Argument] in h2 shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: celsius [Argument] in KTF shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: cai [Argument] in h2 shadows definition in NMODL_GLOBAL [NMODL] [warning] :: Can not inline function call to alp [NMODL] [warning] :: Can not inline function call to alp [NMODL] [warning] :: Can not inline function call to exptable [NMODL] [warning] :: Can not inline function call to exptable [NMODL] [warning] :: Can not inline function call to exptable [NMODL] [warning] :: SYMTAB :: t [Argument] in init_sequence shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: ca [Argument] in rate shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: ca [Argument] in rate shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: ca [Argument] in rate shadows definition in NMODL_GLOBAL [NMODL] [warning] :: CVode solver of icur in 64.20-30 replaced with cnexp solver [NMODL] [warning] :: SYMTAB :: mggate [LocalVar] in StatementBlock1 shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: mggate [LocalVar] in StatementBlock18 shadows definition in NMODL_GLOBAL [NMODL] [warning] :: SYMTAB :: itot_nmda [LocalVar] in StatementBlock1 shadows definition in NMODL_GLOBAL ... ``` - Use fmt instead of string concatenation (#1351) Co-authored-by: Nicolas Cornu --- src/symtab/symbol_table.cpp | 23 ++++++++++++----------- src/visitors/inline_visitor.cpp | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/symtab/symbol_table.cpp b/src/symtab/symbol_table.cpp index 638141f08..06dfab5a5 100644 --- a/src/symtab/symbol_table.cpp +++ b/src/symtab/symbol_table.cpp @@ -192,10 +192,7 @@ std::shared_ptr ModelSymbolTable::lookup(const std::string& name) { } -/** - * Emit warning message for shadowing definition or throw an exception - * if variable is being redefined in the same block. - */ +// show symbol table related message for debugging purposes void ModelSymbolTable::emit_message(const std::shared_ptr& first, const std::shared_ptr& second, bool redefinition) { @@ -209,15 +206,19 @@ void ModelSymbolTable::emit_message(const std::shared_ptr& first, } if (redefinition) { - std::string msg = "Re-declaration of " + name + " [" + type + "]"; - msg += "<" + properties + "> in " + current_symtab->name(); - msg += " with one in " + second->get_scope(); + auto msg = fmt::format("Re-declaration of {} [{}] <{}> in {} with one in {}", + name, + type, + properties, + current_symtab->name(), + second->get_scope()); throw std::runtime_error(msg); } - std::string msg = "SYMTAB :: " + name + " [" + type + "] in "; - msg += current_symtab->name() + " shadows <" + properties; - msg += "> definition in " + second->get_scope(); - logger->warn(msg); + logger->debug("SYMTAB :: {} [{}] in {} shadows <{}> definition in {}", + name, + type, + current_symtab->name(), + properties); } diff --git a/src/visitors/inline_visitor.cpp b/src/visitors/inline_visitor.cpp index 1dd35629f..5678395ee 100644 --- a/src/visitors/inline_visitor.cpp +++ b/src/visitors/inline_visitor.cpp @@ -132,7 +132,7 @@ bool InlineVisitor::inline_function_call(ast::Block& callee, /// do nothing if we can't inline given procedure/function if (!can_inline_block(*callee.get_statement_block())) { - logger->warn("Can not inline function call to {}", function_name); + logger->debug("Can not inline function call to {}", function_name); return false; } From 9b59909e0eaeb8fef77002ee471bba7e7b05f9d2 Mon Sep 17 00:00:00 2001 From: JCGoran Date: Tue, 16 Jul 2024 20:09:39 +0200 Subject: [PATCH 09/18] Remove usage of `unistd` from Flex (#1348) --- src/lexer/verbatim.l | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lexer/verbatim.l b/src/lexer/verbatim.l index d2e13764c..79e1fba5e 100755 --- a/src/lexer/verbatim.l +++ b/src/lexer/verbatim.l @@ -49,6 +49,9 @@ */ %option yymore +%option nounistd +%option never-interactive + /** lexer header file */ %option header-file="verbatim_lexer.hpp" From 8052b40cca7061377d607f0db985bec20b3a3412 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Wed, 17 Jul 2024 10:13:53 +0200 Subject: [PATCH 10/18] Simplify internal code. (#1347) --- src/codegen/codegen_neuron_cpp_visitor.cpp | 38 ++++++++++------------ 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index bea1f104e..bf1f39625 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -1014,27 +1014,23 @@ void CodegenNeuronCppVisitor::print_global_param_default_values() { printer->pop_block(";"); } -/// TODO: Same as CoreNEURON? void CodegenNeuronCppVisitor::print_global_variables_for_hoc() { - /// TODO: Write HocParmLimits and other HOC global variables (delta_t) - // Probably needs more changes - auto variable_printer = - [&](const std::vector& variables, bool if_array, bool if_vector) { - for (const auto& variable: variables) { - if (variable->is_array() == if_array) { - // false => do not use the instance struct, which is not - // defined in the global declaration that we are printing - auto name = get_variable_name(variable->get_name(), false); - auto ename = add_escape_quote(variable->get_name() + "_" + info.mod_suffix); + auto variable_printer = [&](const std::vector& variables, bool if_array) { + for (const auto& variable: variables) { + if (variable->is_array() == if_array) { + // false => do not use the instance struct, which is not + // defined in the global declaration that we are printing + auto name = get_variable_name(variable->get_name(), false); + auto ename = add_escape_quote(variable->get_name() + "_" + info.mod_suffix); + if (if_array) { auto length = variable->get_length(); - if (if_vector) { - printer->fmt_line("{{{}, {}, {}}},", ename, name, length); - } else { - printer->fmt_line("{{{}, &{}}},", ename, name); - } + printer->fmt_line("{{{}, {}, {}}},", ename, name, length); + } else { + printer->fmt_line("{{{}, &{}}},", ename, name); } } - }; + } + }; auto globals = info.global_variables; auto thread_vars = info.thread_variables; @@ -1047,8 +1043,8 @@ void CodegenNeuronCppVisitor::print_global_variables_for_hoc() { printer->add_line("/** connect global (scalar) variables to hoc -- */"); printer->add_line("static DoubScal hoc_scalar_double[] = {"); printer->increase_indent(); - variable_printer(globals, false, false); - variable_printer(thread_vars, false, false); + variable_printer(globals, false); + variable_printer(thread_vars, false); printer->add_line("{nullptr, nullptr}"); printer->decrease_indent(); printer->add_line("};"); @@ -1057,8 +1053,8 @@ void CodegenNeuronCppVisitor::print_global_variables_for_hoc() { printer->add_line("/** connect global (array) variables to hoc -- */"); printer->add_line("static DoubVec hoc_vector_double[] = {"); printer->increase_indent(); - variable_printer(globals, true, true); - variable_printer(thread_vars, true, true); + variable_printer(globals, true); + variable_printer(thread_vars, true); printer->add_line("{nullptr, nullptr, 0}"); printer->decrease_indent(); printer->add_line("};"); From 4f47074140fb810a09847c3879c9566c57e4abd9 Mon Sep 17 00:00:00 2001 From: Matthias Wolf Date: Wed, 17 Jul 2024 23:04:08 +0200 Subject: [PATCH 11/18] Smaller changes for MSVC compatibility. (#1355) - These fixes come from #1354, and should not be controversial. - Generated sources are already split into dir, filename --- src/codegen/codegen_cpp_visitor.cpp | 2 +- src/language/code_generator.py | 2 +- src/language/templates/code_generator.cmake | 6 +++--- src/parser/diffeq_context.cpp | 2 +- src/visitors/inline_visitor.hpp | 1 + src/visitors/local_var_rename_visitor.hpp | 1 + src/visitors/localize_visitor.hpp | 1 + src/visitors/sympy_conductance_visitor.hpp | 1 + 8 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp index 611ff26b4..902648866 100644 --- a/src/codegen/codegen_cpp_visitor.cpp +++ b/src/codegen/codegen_cpp_visitor.cpp @@ -1353,7 +1353,7 @@ void CodegenCppVisitor::setup(const Program& node) { info.mod_file = mod_filename; if (info.mod_suffix == "") { - info.mod_suffix = std::filesystem::path(mod_filename).stem(); + info.mod_suffix = std::filesystem::path(mod_filename).stem().string(); } info.rsuffix = info.point_process ? "" : "_" + info.mod_suffix; diff --git a/src/language/code_generator.py b/src/language/code_generator.py index e160039ed..23bf8ed1d 100644 --- a/src/language/code_generator.py +++ b/src/language/code_generator.py @@ -110,7 +110,7 @@ def jinja_template(self, path): Returns: A jinja template object """ - name = str(path.relative_to(self.jinja_templates_dir)) + name = str(path.relative_to(self.jinja_templates_dir).as_posix()) return self.jinja_env.get_template(name) def _cmake_deps_task(self, tasks): diff --git a/src/language/templates/code_generator.cmake b/src/language/templates/code_generator.cmake index 4e462fe48..45d150bc1 100644 --- a/src/language/templates/code_generator.cmake +++ b/src/language/templates/code_generator.cmake @@ -5,19 +5,19 @@ # cmake-format: off set(CODE_GENERATOR_JINJA_FILES {% for template in templates | sort %} - ${PROJECT_SOURCE_DIR}/src/language/templates/{{ template }} + ${PROJECT_SOURCE_DIR}/src/language/templates/{{ template.as_posix() }} {% endfor %} ) set(CODE_GENERATOR_PY_FILES {% for file in py_files | sort %} - ${PROJECT_SOURCE_DIR}/src/language/{{ file }} + ${PROJECT_SOURCE_DIR}/src/language/{{ file.as_posix() }} {% endfor %} ) set(CODE_GENERATOR_YAML_FILES {% for file in yaml_files | sort %} - ${PROJECT_SOURCE_DIR}/src/language/{{ file }} + ${PROJECT_SOURCE_DIR}/src/language/{{ file.as_posix() }} {% endfor %} ) diff --git a/src/parser/diffeq_context.cpp b/src/parser/diffeq_context.cpp index 298b2185d..24dbbd8c7 100644 --- a/src/parser/diffeq_context.cpp +++ b/src/parser/diffeq_context.cpp @@ -151,7 +151,7 @@ std::string DiffEqContext::get_non_cnexp_solution() const { std::string result; if (!deriv_invalid) { result = get_cvode_linear_diffeq(); - } else if (deriv_invalid and eqn_invalid) { + } else if (deriv_invalid && eqn_invalid) { result = get_cvode_nonlinear_diffeq(); } else { throw std::runtime_error("Error in differential equation solver with non-cnexp"); diff --git a/src/visitors/inline_visitor.hpp b/src/visitors/inline_visitor.hpp index 9a728da04..e934b3159 100644 --- a/src/visitors/inline_visitor.hpp +++ b/src/visitors/inline_visitor.hpp @@ -14,6 +14,7 @@ #include #include +#include #include "symtab/decl.hpp" #include "visitors/ast_visitor.hpp" diff --git a/src/visitors/local_var_rename_visitor.hpp b/src/visitors/local_var_rename_visitor.hpp index a73196e19..4b46953e9 100644 --- a/src/visitors/local_var_rename_visitor.hpp +++ b/src/visitors/local_var_rename_visitor.hpp @@ -14,6 +14,7 @@ #include #include +#include #include "symtab/decl.hpp" #include "visitors/ast_visitor.hpp" diff --git a/src/visitors/localize_visitor.hpp b/src/visitors/localize_visitor.hpp index 821e072fe..4dcd99839 100644 --- a/src/visitors/localize_visitor.hpp +++ b/src/visitors/localize_visitor.hpp @@ -13,6 +13,7 @@ */ #include +#include #include "symtab/decl.hpp" #include "visitors/ast_visitor.hpp" diff --git a/src/visitors/sympy_conductance_visitor.hpp b/src/visitors/sympy_conductance_visitor.hpp index cb76bdf2f..d21732029 100644 --- a/src/visitors/sympy_conductance_visitor.hpp +++ b/src/visitors/sympy_conductance_visitor.hpp @@ -14,6 +14,7 @@ #include #include +#include #include #include "visitors/ast_visitor.hpp" From 38f90fff816e8124a42f16275fbd1add0fa5181a Mon Sep 17 00:00:00 2001 From: Matthias Wolf Date: Fri, 19 Jul 2024 10:05:13 +0200 Subject: [PATCH 12/18] Implement CI to check for STL misuse (#1358) Compiles and runs NMODL with glibc debug macros enabled to check the inputs to STL algorithms etc. --- .github/workflows/nmodl-ci.yml | 16 ++++++++++------ azure-pipelines.yml | 2 +- src/visitors/semantic_analysis_visitor.cpp | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/.github/workflows/nmodl-ci.yml b/.github/workflows/nmodl-ci.yml index 70e3f8cf8..ec71cc692 100644 --- a/.github/workflows/nmodl-ci.yml +++ b/.github/workflows/nmodl-ci.yml @@ -30,6 +30,7 @@ jobs: os: ubuntu-20.04 - config: flag_warnings: ON + glibc_asserts: ON os: ubuntu-22.04 - config: os: macos-12 @@ -113,7 +114,6 @@ jobs: working-directory: ${{runner.workspace}}/nmodl run: | echo "------- Configure -------" - mkdir build && pushd build cmake_args=(-G Ninja -DPYTHON_EXECUTABLE=$(which python3) \ -DCMAKE_INSTALL_PREFIX=$INSTALL_DIR) @@ -134,11 +134,15 @@ jobs: CXX=$(command -v clang++-14) else CXX=${CXX:-g++} + if [[ -n "${{matrix.config.glibc_asserts}}" ]]; then + cmake_args+=(-DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_CXX_FLAGS=-D_GLIBCXX_DEBUG) + fi fi cmake_args+=(-DCMAKE_CXX_COMPILER=${CXX} \ -DCMAKE_CXX_COMPILER_LAUNCHER=ccache) cmake_args+=(-DNMODL_ENABLE_BACKWARD=On) - cmake .. "${cmake_args[@]}" + cmake -B build -S . "${cmake_args[@]}" env: INSTALL_DIR: ${{ runner.workspace }}/install @@ -164,7 +168,7 @@ jobs: - name: Build shell: bash - working-directory: ${{runner.workspace}}/nmodl/build + working-directory: ${{runner.workspace}}/nmodl env: CCACHE_BASEDIR: ${{runner.workspace}}/nmodl CCACHE_DIR: ${{runner.workspace}}/ccache @@ -180,7 +184,7 @@ jobs: ccache -z # Older versions don't support -v (verbose) ccache -vs 2>/dev/null || ccache -s - cmake --build . + cmake --build build ccache -vs 2>/dev/null || ccache -s - name: Test shell: bash @@ -191,9 +195,9 @@ jobs: ctest --output-on-failure -T Test - name: Install shell: bash - working-directory: ${{runner.workspace}}/nmodl/build + working-directory: ${{runner.workspace}}/nmodl run: | - cmake --build . --target install + cmake --install build - name: Update references if: matrix.config.update_references == 'On' diff --git a/azure-pipelines.yml b/azure-pipelines.yml index fc32c71da..7b456ef3a 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -148,7 +148,7 @@ stages: submodules: True - script: | brew install flex bison cmake python@3 - python3 -m pip install --upgrade pip setuptools + python3 -m pip install --upgrade pip "setuptools<=70.3.0" python3 -m pip install --user -r requirements.txt displayName: 'Install Dependencies' - script: | diff --git a/src/visitors/semantic_analysis_visitor.cpp b/src/visitors/semantic_analysis_visitor.cpp index 6d005e138..a2f852664 100644 --- a/src/visitors/semantic_analysis_visitor.cpp +++ b/src/visitors/semantic_analysis_visitor.cpp @@ -56,14 +56,14 @@ bool SemanticAnalysisVisitor::check_table_vars(const ast::Program& node) { bool SemanticAnalysisVisitor::check_name_conflict(const ast::Program& node) { // check that there are no RANGE variables which have the same name as a FUNCTION or PROCEDURE const auto& range_nodes = collect_nodes(node, {ast::AstNodeType::RANGE_VAR}); - std::unordered_set range_vars{}; + std::set range_vars{}; for (const auto& range_node: range_nodes) { range_vars.insert(range_node->get_node_name()); } const auto& function_nodes = collect_nodes(node, {ast::AstNodeType::FUNCTION_BLOCK, ast::AstNodeType::PROCEDURE_BLOCK}); - std::unordered_set func_vars{}; + std::set func_vars{}; for (const auto& function_node: function_nodes) { func_vars.insert(function_node->get_node_name()); } From 9ae0783dadd5cef8bd6b0f7b50d7498e1ab086e9 Mon Sep 17 00:00:00 2001 From: Matthias Wolf Date: Mon, 22 Jul 2024 08:38:23 +0200 Subject: [PATCH 13/18] Provide both end() iterators to std::mismatch to prevent seeking beyond the end of std::string (#1357) Became apparent with MSVC, which in this case crashes hard comparing `it.first = "yotta"` and `denomitator_name = "sec2"`. --- src/units/units.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/units/units.cpp b/src/units/units.cpp index 5f1465ab2..c1f391bfa 100644 --- a/src/units/units.cpp +++ b/src/units/units.cpp @@ -132,7 +132,10 @@ void UnitTable::calc_nominator_dims(const std::shared_ptr& unit, std::stri while (changed_nominator_name) { changed_nominator_name = 0; for (const auto& it: prefixes) { - auto res = std::mismatch(it.first.begin(), it.first.end(), nominator_name.begin()); + auto res = std::mismatch(it.first.begin(), + it.first.end(), + nominator_name.begin(), + nominator_name.end()); if (res.first == it.first.end()) { changed_nominator_name = 1; nominator_prefix_factor *= it.second; @@ -207,8 +210,10 @@ void UnitTable::calc_denominator_dims(const std::shared_ptr& unit, while (changed_denominator_name) { changed_denominator_name = false; for (const auto& it: prefixes) { - auto res = - std::mismatch(it.first.begin(), it.first.end(), denominator_name.begin()); + auto res = std::mismatch(it.first.begin(), + it.first.end(), + denominator_name.begin(), + denominator_name.end()); if (res.first == it.first.end()) { changed_denominator_name = true; denominator_prefix_factor *= it.second; From 3f8da3a52d60ba91fc39bf4eded70f97049f4693 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Mon, 22 Jul 2024 13:38:28 +0200 Subject: [PATCH 14/18] Accept negative number for limits (#1352) * Accept negative number for limits * Add regression test. --------- Co-authored-by: Luc Grosheintz --- src/language/nmodl.yaml | 4 ++-- src/parser/nmodl.yy | 2 +- test/usecases/parameter/limits.mod | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 test/usecases/parameter/limits.mod diff --git a/src/language/nmodl.yaml b/src/language/nmodl.yaml index 60919df1b..7895ba1c1 100644 --- a/src/language/nmodl.yaml +++ b/src/language/nmodl.yaml @@ -1027,12 +1027,12 @@ members: - min: brief: "TODO" - type: Double + type: Number prefix: {value: "<"} suffix: {value: ","} - max: brief: "TODO" - type: Double + type: Number suffix: {value: ">"} - NumberRange: diff --git a/src/parser/nmodl.yy b/src/parser/nmodl.yy index c07bd961a..8d910e0ca 100644 --- a/src/parser/nmodl.yy +++ b/src/parser/nmodl.yy @@ -588,7 +588,7 @@ unit_state : UNITSON limits : { $$ = nullptr; } - | LT double "," double GT + | LT number "," number GT { $$ = new ast::Limits($2, $4); } diff --git a/test/usecases/parameter/limits.mod b/test/usecases/parameter/limits.mod new file mode 100644 index 000000000..23bd6e2dd --- /dev/null +++ b/test/usecases/parameter/limits.mod @@ -0,0 +1,14 @@ +: Only test if all of these result in compilable code. + +NEURON { + SUFFIX limits_mod +} + +PARAMETER { + a = 24. (mV) <-2, 3> + b = 24. (mV) <-2.1, 3> + c = 24. (mV) <-4.1,-3> + x = 24. (mV) < -2, 3> + y = 24. (mV) < -2.1, 3> + z = 24. (mV) < -4.1, -3> +} From 7face1d5fdc8d5e109d44cea15e27b7d44156fc9 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 22 Jul 2024 14:55:52 +0200 Subject: [PATCH 15/18] Deduplicate integer magic code. (#1362) * Deduplicate code for finding the index given the name. * Deduplicate computing prefixsum. * Deduplicate `get_int_variable_index`. --- .../codegen_coreneuron_cpp_visitor.cpp | 18 +------- src/codegen/codegen_cpp_visitor.cpp | 12 +----- src/codegen/codegen_cpp_visitor.hpp | 41 +++++++++++++++++++ src/codegen/codegen_neuron_cpp_visitor.cpp | 21 +--------- 4 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/codegen/codegen_coreneuron_cpp_visitor.cpp b/src/codegen/codegen_coreneuron_cpp_visitor.cpp index 74581bbaa..b477a5891 100644 --- a/src/codegen/codegen_coreneuron_cpp_visitor.cpp +++ b/src/codegen/codegen_coreneuron_cpp_visitor.cpp @@ -66,26 +66,12 @@ std::string CodegenCoreneuronCppVisitor::simulator_name() { int CodegenCoreneuronCppVisitor::position_of_float_var(const std::string& name) const { - int index = 0; - for (const auto& var: codegen_float_variables) { - if (var->get_name() == name) { - return index; - } - index += var->get_length(); - } - throw std::logic_error(name + " variable not found"); + return get_prefixsum_from_name(codegen_float_variables, name); } int CodegenCoreneuronCppVisitor::position_of_int_var(const std::string& name) const { - int index = 0; - for (const auto& var: codegen_int_variables) { - if (var.symbol->get_name() == name) { - return index; - } - index += var.symbol->get_length(); - } - throw std::logic_error(name + " variable not found"); + return get_prefixsum_from_name(codegen_int_variables, name); } diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp index 902648866..63eb9f3bc 100644 --- a/src/codegen/codegen_cpp_visitor.cpp +++ b/src/codegen/codegen_cpp_visitor.cpp @@ -404,17 +404,7 @@ std::pair CodegenCppVisitor::write_ion_variable_name( int CodegenCppVisitor::get_int_variable_index(const std::string& var_name) { - auto it = std::find_if(codegen_int_variables.cbegin(), - codegen_int_variables.cend(), - [&var_name](const auto& int_var) { - return int_var.symbol->get_name() == var_name; - }); - - if (it == codegen_int_variables.cend()) { - throw std::runtime_error(fmt::format("Unknown int variable: {}", var_name)); - } - - return static_cast(it - codegen_int_variables.cbegin()); + return get_index_from_name(codegen_int_variables, var_name); } diff --git a/src/codegen/codegen_cpp_visitor.hpp b/src/codegen/codegen_cpp_visitor.hpp index 8a1415c23..7c24a3f6d 100644 --- a/src/codegen/codegen_cpp_visitor.hpp +++ b/src/codegen/codegen_cpp_visitor.hpp @@ -169,6 +169,47 @@ struct ShadowUseStatement { std::string rhs; }; +inline std::string get_name(const std::shared_ptr& sym) { + return sym->get_name(); +} + +inline std::string get_name(const IndexVariableInfo& var) { + return var.symbol->get_name(); +} + +template +int get_index_from_name(const std::vector& variables, const std::string& name) { + auto it = std::find_if(variables.cbegin(), variables.cend(), [&name](const auto& var) { + return get_name(var) == name; + }); + + if (it == variables.cend()) { + throw std::runtime_error(fmt::format("Unknown variable: {}", name)); + } + + return static_cast(it - variables.cbegin()); +} + +inline int get_length(const std::shared_ptr& sym) { + return sym->get_length(); +} + +inline int get_length(const IndexVariableInfo& var) { + return var.symbol->get_length(); +} + +template +int get_prefixsum_from_name(const std::vector& variables, const std::string& name) { + int index = 0; + for (const auto& var: variables) { + if (get_name(var) == name) { + return index; + } + index += get_length(var); + } + throw std::logic_error(name + " variable not found"); +} + /** \} */ // end of codegen_details diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index bf1f39625..fc7a3d69e 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -61,30 +61,13 @@ std::string CodegenNeuronCppVisitor::table_thread_function_name() const { /* Common helper routines accross codegen functions */ /****************************************************************************************/ - int CodegenNeuronCppVisitor::position_of_float_var(const std::string& name) const { - const auto has_name = [&name](const SymbolType& symbol) { return symbol->get_name() == name; }; - const auto var_iter = - std::find_if(codegen_float_variables.begin(), codegen_float_variables.end(), has_name); - if (var_iter != codegen_float_variables.end()) { - return var_iter - codegen_float_variables.begin(); - } else { - throw std::logic_error(name + " variable not found"); - } + return get_index_from_name(codegen_float_variables, name); } int CodegenNeuronCppVisitor::position_of_int_var(const std::string& name) const { - const auto has_name = [&name](const IndexVariableInfo& index_var_symbol) { - return index_var_symbol.symbol->get_name() == name; - }; - const auto var_iter = - std::find_if(codegen_int_variables.begin(), codegen_int_variables.end(), has_name); - if (var_iter != codegen_int_variables.end()) { - return var_iter - codegen_int_variables.begin(); - } else { - throw std::logic_error(name + " variable not found"); - } + return get_index_from_name(codegen_int_variables, name); } From c2320b809259b82120979519f7459b7d984381c7 Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 22 Jul 2024 14:56:28 +0200 Subject: [PATCH 16/18] Fix `net_send` and `net_move` for ARTIFICIAL_CELLs. (#1360) * Fix `net_send` for ARTIFICIAL_CELLs. * Fix `net_move` for ARTIFICIAL_CELL. * Add tests. --- src/codegen/codegen_neuron_cpp_visitor.cpp | 8 +++-- .../codegen/codegen_neuron_cpp_visitor.cpp | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/codegen/codegen_neuron_cpp_visitor.cpp b/src/codegen/codegen_neuron_cpp_visitor.cpp index fc7a3d69e..3ee005361 100644 --- a/src/codegen/codegen_neuron_cpp_visitor.cpp +++ b/src/codegen/codegen_neuron_cpp_visitor.cpp @@ -2139,7 +2139,8 @@ void CodegenNeuronCppVisitor::print_net_send_call(const ast::FunctionCall& node) } const auto& tqitem = get_variable_name("tqitem", /* use_instance */ false); - printer->fmt_text("net_send(/* tqitem */ &{}, {}, {}, {} + ", + printer->fmt_text("{}(/* tqitem */ &{}, {}, {}, {} + ", + info.artificial_cell ? "artcell_net_send" : "net_send", tqitem, weight_pointer, point_process, @@ -2152,7 +2153,10 @@ void CodegenNeuronCppVisitor::print_net_move_call(const ast::FunctionCall& node) const auto& point_process = get_variable_name("point_process", /* use_instance */ false); const auto& tqitem = get_variable_name("tqitem", /* use_instance */ false); - printer->fmt_text("net_move(/* tqitem */ &{}, {}, ", tqitem, point_process); + printer->fmt_text("{}(/* tqitem */ &{}, {}, ", + info.artificial_cell ? "artcell_net_move" : "net_move", + tqitem, + point_process); print_vector_elements(node.get_arguments(), ", "); printer->add_text(')'); diff --git a/test/unit/codegen/codegen_neuron_cpp_visitor.cpp b/test/unit/codegen/codegen_neuron_cpp_visitor.cpp index 2189ed076..8268eb22d 100644 --- a/test/unit/codegen/codegen_neuron_cpp_visitor.cpp +++ b/test/unit/codegen/codegen_neuron_cpp_visitor.cpp @@ -209,3 +209,39 @@ SCENARIO("Read `eca`.", "[codegen]") { } } } + +SCENARIO("ARTIFICIAL_CELL with `net_send`") { + GIVEN("a mod file") { + std::string nmodl = R"( + NEURON { + ARTIFICIAL_CELL test + } + NET_RECEIVE(w) { + net_send(t+1, 1) + } + )"; + std::string cpp = transpile(nmodl); + + THEN("it contains") { + REQUIRE_THAT(cpp, ContainsSubstring("artcell_net_send")); + } + } +} + +SCENARIO("ARTIFICIAL_CELL with `net_move`") { + GIVEN("a mod file") { + std::string nmodl = R"( + NEURON { + ARTIFICIAL_CELL test + } + NET_RECEIVE(w) { + net_move(t+1) + } + )"; + std::string cpp = transpile(nmodl); + + THEN("it contains") { + REQUIRE_THAT(cpp, ContainsSubstring("artcell_net_move")); + } + } +} From 2f0b280cc860d5fad606514fc2e05a32ad5bf33e Mon Sep 17 00:00:00 2001 From: Luc Grosheintz Date: Mon, 22 Jul 2024 14:56:55 +0200 Subject: [PATCH 17/18] Simplify by using structured binding. (#1361) --- src/codegen/codegen_cpp_visitor.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/codegen/codegen_cpp_visitor.cpp b/src/codegen/codegen_cpp_visitor.cpp index 63eb9f3bc..c65408fc0 100644 --- a/src/codegen/codegen_cpp_visitor.cpp +++ b/src/codegen/codegen_cpp_visitor.cpp @@ -463,9 +463,7 @@ void CodegenCppVisitor::print_function_call(const FunctionCall& node) { } return {name, false}; }; - std::string function_name; - bool is_random_function; - std::tie(function_name, is_random_function) = get_renamed_random_function(name); + auto [function_name, is_random_function] = get_renamed_random_function(name); if (defined_method(name)) { function_name = method_name(name); From b91c0eae37bb26d97300471f2057874b429a7bec Mon Sep 17 00:00:00 2001 From: Matthias Wolf Date: Mon, 22 Jul 2024 14:57:31 +0200 Subject: [PATCH 18/18] Build with MSVC (#1354) Compilation works for me with: cmake -B build -S . -DFLEX_INCLUDE_PATH=C:\ProgramData\chocolatey\lib\winflexbison3\tools -DBUILD_SHARED_LIBS=OFF I used choco to install winflexbison, the remaining dependencies came from winget. Shared libraries should be disabled, otherwise dl, fmt and spdlog leave DLLs scattered around and the Python module will need additional search paths. --- .github/workflows/nmodl-ci.yml | 29 +++++++++++++++++++++++++++-- .gitmodules | 3 +++ CMakeLists.txt | 5 +++++ cmake/FlexHelper.cmake | 6 ++++-- ext/dlfcn-win32 | 1 + pyproject.toml | 3 +++ src/lexer/main_c.cpp | 6 ++++-- src/lexer/main_nmodl.cpp | 18 ++++++++++-------- src/parser/main_c.cpp | 3 ++- src/pybind/CMakeLists.txt | 15 ++++++++++++++- src/pybind/pyembed.cpp | 3 ++- src/pybind/wrapper.cpp | 2 +- src/pybind/wrapper.hpp | 8 +++++++- test/integration/CMakeLists.txt | 8 ++++++-- test/unit/CMakeLists.txt | 12 ++++++++---- test/unit/newton/newton.cpp | 18 +++++++++--------- 16 files changed, 106 insertions(+), 34 deletions(-) create mode 160000 ext/dlfcn-win32 diff --git a/.github/workflows/nmodl-ci.yml b/.github/workflows/nmodl-ci.yml index ec71cc692..907028796 100644 --- a/.github/workflows/nmodl-ci.yml +++ b/.github/workflows/nmodl-ci.yml @@ -36,6 +36,8 @@ jobs: os: macos-12 - config: os: macos-13 + - config: + os: windows-latest # TODO: might be interesting to add the thread sanitizer too - config: os: ubuntu-22.04 @@ -83,6 +85,15 @@ jobs: echo CMAKE_BUILD_PARALLEL_LEVEL=2 >> $GITHUB_ENV shell: bash + - name: Install choco packages + if: startsWith(matrix.config.os, 'windows') + run: | + choco install winflexbison3 + + - name: Configure VS Toolchain (Windows) + if: startsWith(matrix.config.os, 'windows') + uses: ilammy/msvc-dev-cmd@v1 + - name: Set up Python3 uses: actions/setup-python@v5 with: @@ -109,7 +120,13 @@ jobs: if: ${{matrix.config.sanitizer}} run: echo "::add-matcher::.github/problem-matchers/${{matrix.config.sanitizer}}.json" - - name: Configure + - name: Configure Windows + if: startsWith(matrix.config.os, 'windows') + run: | + cmake -S . -B build -DFLEX_INCLUDE_PATH=C:/ProgramData/chocolatey/lib/winflexbison3/tools -DBUILD_SHARED_LIBS=OFF + + - name: Configure Linux/MacOS + if: ${{ !startsWith(matrix.config.os, 'windows') }} shell: bash working-directory: ${{runner.workspace}}/nmodl run: | @@ -147,6 +164,7 @@ jobs: INSTALL_DIR: ${{ runner.workspace }}/install - name: Dump config dictionary + if: ${{ !startsWith(matrix.config.os, 'windows') }} run: | cat << EOF > matrix.json ${{toJSON(matrix.config)}} @@ -156,6 +174,7 @@ jobs: echo ----- - name: Restore compiler cache + if: ${{ !startsWith(matrix.config.os, 'windows') }} uses: actions/cache@v4 with: path: | @@ -166,7 +185,13 @@ jobs: ${{hashfiles('matrix.json')}}- save-always: true - - name: Build + - name: Build Windows + if: startsWith(matrix.config.os, 'windows') + run: | + cmake --build build + + - name: Build Linux/MacOS + if: ${{ !startsWith(matrix.config.os, 'windows') }} shell: bash working-directory: ${{runner.workspace}}/nmodl env: diff --git a/.gitmodules b/.gitmodules index f9af32cc5..97e533bbc 100644 --- a/.gitmodules +++ b/.gitmodules @@ -25,3 +25,6 @@ [submodule "ext/backward"] path = ext/backward url = https://github.com/bombela/backward-cpp.git +[submodule "ext/dlfcn-win32"] + path = ext/dlfcn-win32 + url = https://github.com/dlfcn-win32/dlfcn-win32 diff --git a/CMakeLists.txt b/CMakeLists.txt index 1247d866a..84e01c07a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -130,6 +130,11 @@ if(NOT NMODL_3RDPARTY_USE_FMT endif() cpp_cc_git_submodule(json BUILD PACKAGE nlohmann_json REQUIRED) cpp_cc_git_submodule(pybind11 BUILD PACKAGE pybind11 REQUIRED) +if(WIN32) + cpp_cc_git_submodule(dlfcn-win32 BUILD) + add_library(dlfcn-win32::dl ALIAS dl) + set(CMAKE_DL_LIBS dlfcn-win32::dl) +endif() # Tell spdlog not to use its bundled fmt, it should either use the fmt submodule or a truly external # 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) diff --git a/cmake/FlexHelper.cmake b/cmake/FlexHelper.cmake index 6b7c2bc75..a6c75818e 100644 --- a/cmake/FlexHelper.cmake +++ b/cmake/FlexHelper.cmake @@ -6,8 +6,10 @@ get_filename_component(FLEX_BIN_DIR ${FLEX_EXECUTABLE} DIRECTORY) if(NOT FLEX_BIN_DIR MATCHES "/usr/bin") - get_filename_component(FLEX_INCLUDE_PATH ${FLEX_BIN_DIR} PATH) - set(FLEX_INCLUDE_PATH ${FLEX_INCLUDE_PATH}/include/) + if(NOT FLEX_INCLUDE_PATH) + get_filename_component(FLEX_INCLUDE_PATH ${FLEX_BIN_DIR} PATH) + set(FLEX_INCLUDE_PATH ${FLEX_INCLUDE_PATH}/include/) + endif() if(EXISTS "${FLEX_INCLUDE_PATH}/FlexLexer.h") message(STATUS " Adding Flex include path as : ${FLEX_INCLUDE_PATH}") include_directories(${FLEX_INCLUDE_PATH}) diff --git a/ext/dlfcn-win32 b/ext/dlfcn-win32 new file mode 160000 index 000000000..048bff80f --- /dev/null +++ b/ext/dlfcn-win32 @@ -0,0 +1 @@ +Subproject commit 048bff80f2bd00bb651bcc3357cb6f76e3d76fd5 diff --git a/pyproject.toml b/pyproject.toml index 468e01982..28082e014 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -74,3 +74,6 @@ environment = { PATH = "/nmodlwheel/flex/bin:/nmodlwheel/bison/bin:$PATH" } # the Linux wheel is not tested in cibuildwheel due to manylinux images not having # libpython*.so, so this is tested manually in the CI test-command = "true" + +[tool.cibuildwheel.windows] +environment = { SKBUILD_CMAKE_ARGS = "-DNMODL_BUILD_WHEEL=ON;-DFLEX_INCLUDE_PATH=C:/ProgramData/chocolatey/lib/winflexbison3/tools" } \ No newline at end of file diff --git a/src/lexer/main_c.cpp b/src/lexer/main_c.cpp index 8788af66d..1724554dd 100644 --- a/src/lexer/main_c.cpp +++ b/src/lexer/main_c.cpp @@ -7,13 +7,15 @@ #include -#include - #include "config/config.h" #include "lexer/c11_lexer.hpp" #include "parser/c11_driver.hpp" #include "utils/logger.hpp" +// CLI11 includes Windows specific headers with macros that will interfere with the lexer, include +// it last. +#include + /** * \file * \brief Example of standalone lexer program for C code diff --git a/src/lexer/main_nmodl.cpp b/src/lexer/main_nmodl.cpp index 600570b02..5f75bef2f 100644 --- a/src/lexer/main_nmodl.cpp +++ b/src/lexer/main_nmodl.cpp @@ -26,7 +26,7 @@ * it's value and location. */ -using namespace nmodl; +namespace nmodl { using parser::NmodlDriver; using parser::NmodlLexer; @@ -106,10 +106,12 @@ void tokenize(const std::string& mod_text) { } } +} // namespace nmodl + int main(int argc, const char* argv[]) { - CLI::App app{ - fmt::format("NMODL-Lexer : Standalone Lexer for NMODL Code({})", Version::to_string())}; + CLI::App app{fmt::format("NMODL-Lexer : Standalone Lexer for NMODL Code({})", + nmodl::Version::to_string())}; std::vector mod_files; std::vector mod_texts; @@ -120,16 +122,16 @@ int main(int argc, const char* argv[]) { CLI11_PARSE(app, argc, argv); for (const auto& file: mod_files) { - logger->info("Processing file : {}", file); + nmodl::logger->info("Processing file : {}", file); std::ifstream f(file); std::string mod{std::istreambuf_iterator{f}, {}}; - tokenize(mod); + nmodl::tokenize(mod); } for (const auto& text: mod_texts) { - logger->info("Processing text : {}", text); - tokenize(text); + nmodl::logger->info("Processing text : {}", text); + nmodl::tokenize(text); } return 0; -} +} \ No newline at end of file diff --git a/src/parser/main_c.cpp b/src/parser/main_c.cpp index decc6268c..ab84d7bc0 100644 --- a/src/parser/main_c.cpp +++ b/src/parser/main_c.cpp @@ -8,9 +8,10 @@ #include #include "config/config.h" -#include "parser/c11_driver.hpp" #include "utils/logger.hpp" +#include "parser/c11_driver.hpp" + /** * \file * Standalone parser program for C. This demonstrate basic diff --git a/src/pybind/CMakeLists.txt b/src/pybind/CMakeLists.txt index ecc9447c0..b9d561b94 100644 --- a/src/pybind/CMakeLists.txt +++ b/src/pybind/CMakeLists.txt @@ -24,10 +24,15 @@ set_property( DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/../../python/nmodl/ode.py) +if(WIN32) + # MSVC can't handle long string literals, even if the documentation claims so See + # https://developercommunity.visualstudio.com/t/c-string-literal-max-length-much-shorter-than-docu/758957 + string(REGEX REPLACE "\n\n" "\n)jiowi\" R\"jiowi(\n" NMODL_ODE_PY "${NMODL_ODE_PY}") +endif() configure_file(${CMAKE_CURRENT_SOURCE_DIR}/ode_py.hpp.inc ${CMAKE_CURRENT_BINARY_DIR}/ode_py.hpp @ONLY) -add_library(pyembed pyembed.cpp) +add_library(pyembed STATIC pyembed.cpp) set_property(TARGET pyembed PROPERTY POSITION_INDEPENDENT_CODE ON) target_link_libraries(pyembed PRIVATE util) target_link_libraries(pyembed PRIVATE fmt::fmt) @@ -51,9 +56,11 @@ target_include_directories(pywrapper PRIVATE ${CMAKE_CURRENT_BINARY_DIR}) # pybind11::embed adds PYTHON_LIBRARIES to target_link_libraries. To avoid link to # libpython, we can use `module` interface library from pybind11. # ~~~ +target_link_libraries(pyembed PRIVATE ${CMAKE_DL_LIBS}) if(NOT LINK_AGAINST_PYTHON) target_link_libraries(pywrapper PRIVATE pybind11::module) else() + target_link_libraries(pyembed PRIVATE ${PYTHON_LIBRARIES} pywrapper) target_link_libraries(pywrapper PRIVATE pybind11::embed) endif() @@ -69,6 +76,12 @@ if(NMODL_ENABLE_PYTHON_BINDINGS) ${NMODL_PROJECT_SOURCE_DIR}/src/pybind/pynmodl.cpp ${PYBIND_GENERATED_SOURCES}) add_dependencies(_nmodl lexer pyastgen util) target_link_libraries(_nmodl PRIVATE printer symtab visitor pyembed) + set_target_properties(_nmodl PROPERTIES LIBRARY_OUTPUT_DIRECTORY_DEBUG + ${CMAKE_BINARY_DIR}/lib/nmodl) + + if(MSVC) + target_compile_options(_nmodl PRIVATE /bigobj) + endif() # in case of wheel, python module shouldn't link to wrapper library if(LINK_AGAINST_PYTHON) diff --git a/src/pybind/pyembed.cpp b/src/pybind/pyembed.cpp index 7e4bfb8dc..f6e9b5e49 100644 --- a/src/pybind/pyembed.cpp +++ b/src/pybind/pyembed.cpp @@ -92,7 +92,8 @@ void EmbeddedPythonLoader::load_libraries() { CMakeInfo::SHARED_LIBRARY_SUFFIX); throw std::runtime_error("NMODLHOME doesn't have lib/libpywrapper library"); } - pybind_wrapper_handle = dlopen(pybind_wraplib_env.c_str(), dlopen_opts); + std::string env_str = pybind_wraplib_env.string(); + pybind_wrapper_handle = dlopen(env_str.c_str(), dlopen_opts); if (!pybind_wrapper_handle) { const auto errstr = dlerror(); logger->critical("Tried but failed to load {}", pybind_wraplib_env.string()); diff --git a/src/pybind/wrapper.cpp b/src/pybind/wrapper.cpp index 3c9f2661a..74f4a2cd5 100644 --- a/src/pybind/wrapper.cpp +++ b/src/pybind/wrapper.cpp @@ -193,7 +193,7 @@ void finalize_interpreter_func() { // Prevent mangling for easier `dlsym`. extern "C" { -__attribute__((visibility("default"))) pybind_wrap_api nmodl_init_pybind_wrapper_api() noexcept { +NMODL_EXPORT pybind_wrap_api nmodl_init_pybind_wrapper_api() noexcept { return {&nmodl::pybind_wrappers::initialize_interpreter_func, &nmodl::pybind_wrappers::finalize_interpreter_func, &call_solve_nonlinear_system, diff --git a/src/pybind/wrapper.hpp b/src/pybind/wrapper.hpp index 2a51da014..725f9f811 100644 --- a/src/pybind/wrapper.hpp +++ b/src/pybind/wrapper.hpp @@ -53,8 +53,14 @@ struct pybind_wrap_api { decltype(&call_analytic_diff) analytic_diff; }; +#ifdef _WIN32 +#define NMODL_EXPORT +#else +#define NMODL_EXPORT __attribute__((visibility("default"))) +#endif + extern "C" { -__attribute__((visibility("default"))) pybind_wrap_api nmodl_init_pybind_wrapper_api() noexcept; +pybind_wrap_api nmodl_init_pybind_wrapper_api() noexcept; } diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index eb78fbc1f..3070f1375 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -11,7 +11,11 @@ file(GLOB modfiles CONFIGURE_DEPENDS "${NMODL_PROJECT_SOURCE_DIR}/test/integrati foreach(modfile ${modfiles}) get_filename_component(modfile_name "${modfile}" NAME) add_test(NAME ${modfile_name} COMMAND ${CMAKE_BINARY_DIR}/bin/nmodl ${modfile}) - set_tests_properties(${modfile_name} - PROPERTIES ENVIRONMENT PYTHONPATH=${PROJECT_BINARY_DIR}/lib:$ENV{PYTHONPATH}) + if(WIN32) + set(NMODL_TEST_PYTHONPATH "${PROJECT_BINARY_DIR}/lib;$ENV{PYTHONPATH}") + else() + set(NMODL_TEST_PYTHONPATH "${PROJECT_BINARY_DIR}/lib:$ENV{PYTHONPATH}") + endif() + set_tests_properties(${modfile_name} PROPERTIES ENVIRONMENT "PYTHONPATH=${NMODL_TEST_PYTHONPATH}") cpp_cc_configure_sanitizers(TEST ${modfile_name}) endforeach() diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index a9bf75ee8..184536728 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -172,17 +172,21 @@ endforeach() file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/Unit.inc" "UNITSON \n UNITSOFF \n UNITSON \n UNITSOFF") file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/included.file" "TITLE") +if(WIN32) + set(NMODL_TEST_PYTHONPATH "${PROJECT_BINARY_DIR}/lib;$ENV{PYTHONPATH}") +else() + set(NMODL_TEST_PYTHONPATH "${PROJECT_BINARY_DIR}/lib:$ENV{PYTHONPATH}") +endif() + # ============================================================================= # pybind11 tests # ============================================================================= add_test(NAME Ode COMMAND ${PYTHON_EXECUTABLE} -m pytest ${NMODL_PROJECT_SOURCE_DIR}/test/unit/ode) -set_tests_properties(Ode PROPERTIES ENVIRONMENT - PYTHONPATH=${PROJECT_BINARY_DIR}/lib:$ENV{PYTHONPATH}) +set_tests_properties(Ode PROPERTIES ENVIRONMENT "PYTHONPATH=${NMODL_TEST_PYTHONPATH}") if(NMODL_ENABLE_PYTHON_BINDINGS) add_test(NAME Pybind COMMAND ${PYTHON_EXECUTABLE} -m pytest ${NMODL_PROJECT_SOURCE_DIR}/test/unit/pybind) - set_tests_properties(Pybind PROPERTIES ENVIRONMENT - PYTHONPATH=${PROJECT_BINARY_DIR}/lib:$ENV{PYTHONPATH}) + set_tests_properties(Pybind PROPERTIES ENVIRONMENT "PYTHONPATH=${NMODL_TEST_PYTHONPATH}") cpp_cc_configure_sanitizers(TEST Pybind PRELOAD) endif() diff --git a/test/unit/newton/newton.cpp b/test/unit/newton/newton.cpp index 5b5e0e47e..33f408698 100644 --- a/test/unit/newton/newton.cpp +++ b/test/unit/newton/newton.cpp @@ -29,7 +29,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer Eigen::Matrix X{22.2536}; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<1, functor>(X, fn); fn(X, F); THEN("find the solution") { CAPTURE(iter_newton); @@ -50,7 +50,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer Eigen::Matrix X{-0.21421}; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<1, functor>(X, fn); fn(X, F); THEN("find the solution") { CAPTURE(iter_newton); @@ -72,7 +72,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer Eigen::Matrix X{0.2, 0.4}; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<2, functor>(X, fn); fn(X, F); THEN("find a solution") { CAPTURE(iter_newton); @@ -103,7 +103,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer Eigen::Matrix X{0.21231, 0.4435, -0.11537}; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<3, functor>(X, fn); fn(X, F); THEN("find a solution") { CAPTURE(iter_newton); @@ -131,7 +131,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer Eigen::Matrix X{0.21231, 0.4435, -0.11537, -0.8124312}; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<4, functor>(X, fn); fn(X, F); THEN("find a solution") { CAPTURE(iter_newton); @@ -157,7 +157,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer X << 8.234, -245.46, 123.123, 0.8343, 5.555; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<5, functor>(X, fn); fn(X, F); THEN("find a solution") { CAPTURE(iter_newton); @@ -190,7 +190,7 @@ SCENARIO("Non-linear system to solve with Newton Numerical Diff Solver", "[numer X << 8.234, -5.46, 1.123, 0.8343, 5.555, 18.234, -2.46, 0.123, 10.8343, -4.685; Eigen::Matrix F; functor fn; - int iter_newton = newton::newton_numerical_diff_solver(X, fn); + int iter_newton = newton::newton_numerical_diff_solver<10, functor>(X, fn); fn(X, F); THEN("find a solution") { CAPTURE(iter_newton); @@ -411,7 +411,7 @@ SCENARIO("Non-linear system to solve with Newton Solver", "[analytic][solver]") Eigen::Matrix F; Eigen::Matrix J; functor fn; - int iter_newton = newton::newton_solver(X, fn); + int iter_newton = newton::newton_solver<5, functor>(X, fn); fn(X, F, J); THEN("find a solution") { CAPTURE(iter_newton); @@ -547,7 +547,7 @@ SCENARIO("Non-linear system to solve with Newton Solver", "[analytic][solver]") Eigen::Matrix F; Eigen::Matrix J; functor fn; - int iter_newton = newton::newton_solver(X, fn); + int iter_newton = newton::newton_solver<10, functor>(X, fn); fn(X, F, J); THEN("find a solution") { CAPTURE(iter_newton);