From 90c3948d0c254edb4744af92c7a2acce5e8baaa0 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Tue, 5 Sep 2023 15:17:23 +0200 Subject: [PATCH 1/3] Fix RTD config file (#2217) --- .readthedocs.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.readthedocs.yml b/.readthedocs.yml index f1ba02c781..10cbaca0f2 100644 --- a/.readthedocs.yml +++ b/.readthedocs.yml @@ -15,9 +15,8 @@ formats: [] build: os: ubuntu-22.04 tools: - python: "3.8" + python: "3.11" python: - system_packages: true install: - - requirements: doc/requirements.txt \ No newline at end of file + - requirements: doc/requirements.txt From faeacdd916271b34f8791e2049bdce1c1ccffd9c Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Tue, 19 Sep 2023 10:11:13 +0200 Subject: [PATCH 2/3] Treat some corner cases in modcc (#2216) These tests and fixes cover a few cases related to kinetic blocks: # Non-reaction statement in between reaction statements This leads to the 'normal' statements being hoisted to the top resulting in unexpected semantics. This is now an error. # Reaction statements under conditional This has always been an error; add a test for it. # Reaction/Linear statements in non-KINETIC/LINEAR context This is now an error; did SEGV before. # Derivative statements in KINETIC/LINEAR/PROCEDURE These are now error, they were before, but resulted in a confusing ICE message. --- doc/fileformat/nmodl.rst | 61 ++++++++++++++++++- modcc/expression.cpp | 30 ++++++++- modcc/kineticrewriter.cpp | 1 + .../mod_files/test_alternating_derivative.mod | 21 +++++++ .../test_alternating_differential.mod | 18 ++++++ .../mod_files/test_alternating_reaction.mod | 21 +++++++ .../test_kinetic_under_conditional.mod | 21 +++++++ .../mod_files/test_misplaced_reaction.mod | 15 +++++ test/unit-modcc/test_module.cpp | 50 +++++++++++++++ 9 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 test/unit-modcc/mod_files/test_alternating_derivative.mod create mode 100644 test/unit-modcc/mod_files/test_alternating_differential.mod create mode 100644 test/unit-modcc/mod_files/test_alternating_reaction.mod create mode 100644 test/unit-modcc/mod_files/test_kinetic_under_conditional.mod create mode 100644 test/unit-modcc/mod_files/test_misplaced_reaction.mod diff --git a/doc/fileformat/nmodl.rst b/doc/fileformat/nmodl.rst index 4787c719f9..10b6aa0fcc 100644 --- a/doc/fileformat/nmodl.rst +++ b/doc/fileformat/nmodl.rst @@ -141,7 +141,66 @@ Unsupported features * ``LOCAL`` variables outside blocks are not supported. * free standing blocks are not supported. * ``INDEPENDENT`` variables are not supported. -* arrays and pointers are not supported by Arbor. +* loops, arrays, and pointers are not supported by Arbor. + +Alternating normal and reaction statements in KINETIC +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This is not so much a feature as a bug that Arbor's ``modcc`` does not reproduce +from NEURON. Given a file like: + +.. code-block:: + + NEURON { SUFFIX test_kinetic_alternating_reaction } + + STATE { A B C D } + + BREAKPOINT { + SOLVE foobar METHOD sparse + } + + KINETIC foobar { + LOCAL x, y + + x = 23*v + y = 42*v + + ~ A <-> B (x, y) + + x = sin(y) + y = cos(x) + + ~ C <-> D (x, y) + } + +one might expect that the reaction between ``C`` and ``D`` occurs with a rate +proportional to ``sin(x)`` and ``cos(y)``. However, this file is equivalent to + +.. code-block:: + + NEURON { SUFFIX test_kinetic_alternating_reaction } + + STATE { A B C D } + + BREAKPOINT { + SOLVE foobar METHOD sparse + } + + KINETIC foobar { + LOCAL x, y + + x = 23*v + y = 42*v + x = sin(y) + y = cos(x) + + ~ A <-> B (x, y) + ~ C <-> D (x, y) + } + +which is almost never what the author would expect. Thus, this construction constitutes +an error in ``modcc``; if want this particular behaviour, please state this explicit by +writing code similar to the second example. .. _arbornmodl: diff --git a/modcc/expression.cpp b/modcc/expression.cpp index 82ec3ac679..add6678f7d 100644 --- a/modcc/expression.cpp +++ b/modcc/expression.cpp @@ -555,6 +555,8 @@ void ProcedureExpression::semantic(scope_ptr scp) { error_ = false; scope_ = scp; + auto is_kinetic = [](const auto& it) { return (it->is_reaction() || it->is_conserve()); }; + // assert that the symbol is already visible in the global_symbols if(scope_->find_global(name()) == nullptr) { throw compiler_exception( @@ -571,8 +573,34 @@ void ProcedureExpression::semantic(scope_ptr scp) { // this loop could be used to then check the types of statements in the body for(auto& e : *(body_->is_block())) { - if(e->is_initial_block()) + if (e->is_initial_block()) { error("INITIAL block not allowed inside "+::to_string(kind_)+" definition"); + } + if (kind_ != procedureKind::kinetic && is_kinetic(e)) { + error("reaction statement not allowed inside " + ::to_string(kind_)+" definition"); + } + if (kind_ != procedureKind::linear && e->is_linear()) { + error("linear statement not allowed inside "+::to_string(kind_)+" definition"); + } + if (kind_ != procedureKind::linear && e->is_derivative()) { + error("derivative statement not allowed inside "+::to_string(kind_)+" definition"); + } + } + + // We start a new loop here for preserving our sanity + if (kind_ == procedureKind::kinetic) { + auto it = body_->is_block()->begin(); + auto end = body_->is_block()->end(); + // skip all 'normal' statements + for (; it != end && !is_kinetic(*it); ++it) {} + // skip all 'reaction' statements + for (; it != end && is_kinetic(*it); ++it) {} + // We have trailing 'normal' statements + if (it != end) { + error("Found alternating reaction (A <-> B ...) and normal statements. " + "This is allowed by NMODL, but likely not what you want; see: " + "https://docs.arbor-sim.org/en/latest/fileformat/nmodl.html#unsupported-features"); + } } // perform semantic analysis for each expression in the body diff --git a/modcc/kineticrewriter.cpp b/modcc/kineticrewriter.cpp index b3aef7ab9f..6ad60e76b5 100644 --- a/modcc/kineticrewriter.cpp +++ b/modcc/kineticrewriter.cpp @@ -26,6 +26,7 @@ class KineticRewriter : public BlockRewriterBase { virtual void finalize() override; private: + // Acccumulated terms for derivative expressions, keyed by id name. std::map dterms; }; diff --git a/test/unit-modcc/mod_files/test_alternating_derivative.mod b/test/unit-modcc/mod_files/test_alternating_derivative.mod new file mode 100644 index 0000000000..ee9ce4a02e --- /dev/null +++ b/test/unit-modcc/mod_files/test_alternating_derivative.mod @@ -0,0 +1,21 @@ +NEURON { SUFFIX test_derivative_alternating } + +STATE { A B } + +BREAKPOINT { : NOTE we need a newline here :/ + SOLVE foobar METHOD sparse +} + +: NOTE this is OK to do, while alternating normal and reaction statements is not! + +DERIVATIVE foobar { + LOCAL x + + x = 23*v + + A' = B*x + + x = sin(x) + + B' = x*A +} diff --git a/test/unit-modcc/mod_files/test_alternating_differential.mod b/test/unit-modcc/mod_files/test_alternating_differential.mod new file mode 100644 index 0000000000..dd7e86f8ea --- /dev/null +++ b/test/unit-modcc/mod_files/test_alternating_differential.mod @@ -0,0 +1,18 @@ +NEURON { SUFFIX test_kinetic_alternating_differential } + +STATE { A B C D } + +BREAKPOINT { : NOTE we need a newline here :/ + SOLVE foobar METHOD sparse +} + +KINETIC foobar { + LOCAL x, y + + x = 23*v + y = 42*v + + ~ A <-> B (x, y) + C' = 0.1 + ~ C <-> D (x, y) +} diff --git a/test/unit-modcc/mod_files/test_alternating_reaction.mod b/test/unit-modcc/mod_files/test_alternating_reaction.mod new file mode 100644 index 0000000000..77c9f7a674 --- /dev/null +++ b/test/unit-modcc/mod_files/test_alternating_reaction.mod @@ -0,0 +1,21 @@ +NEURON { SUFFIX test_kinetic_alternating_reaction } + +STATE { A B C D } + +BREAKPOINT { : NOTE we need a newline here :/ + SOLVE foobar METHOD sparse +} + +KINETIC foobar { + LOCAL x, y + + x = 23*v + y = 42*v + + ~ A <-> B (x, y) + + x = sin(y) + y = cos(x) + + ~ C <-> D (x, y) +} diff --git a/test/unit-modcc/mod_files/test_kinetic_under_conditional.mod b/test/unit-modcc/mod_files/test_kinetic_under_conditional.mod new file mode 100644 index 0000000000..50932081a4 --- /dev/null +++ b/test/unit-modcc/mod_files/test_kinetic_under_conditional.mod @@ -0,0 +1,21 @@ +NEURON { SUFFIX test_kinetic_under_conditional } + +STATE { A B } + +BREAKPOINT { : NOTE we need a newline here :/ + SOLVE foobar METHOD sparse +} + +KINETIC foobar { + LOCAL x, y + + x = 23*v + y = 42*v + + if (v<0) { + ~ A <-> B (x, y) + } + else { + ~ A <-> B (y, x) + } +} diff --git a/test/unit-modcc/mod_files/test_misplaced_reaction.mod b/test/unit-modcc/mod_files/test_misplaced_reaction.mod new file mode 100644 index 0000000000..3ab519525d --- /dev/null +++ b/test/unit-modcc/mod_files/test_misplaced_reaction.mod @@ -0,0 +1,15 @@ +NEURON { SUFFIX test_misplaced_reaction } + +STATE { A B } + +BREAKPOINT { : NOTE we need a newline here :/ + SOLVE foobar METHOD sparse +} + +: NOTE this is OK to do, while alternating normal and reaction statements is not! + +DERIVATIVE foobar { + LOCAL x + + ~ A <-> B (x, 1/x) +} diff --git a/test/unit-modcc/test_module.cpp b/test/unit-modcc/test_module.cpp index d57e6c4120..6517fc0d63 100644 --- a/test/unit-modcc/test_module.cpp +++ b/test/unit-modcc/test_module.cpp @@ -216,3 +216,53 @@ TEST(Module, net_receive) { EXPECT_FALSE(m.semantic()); } + +TEST(Module, kinetic_footgun_alternating_reac) { + Module m(io::read_all(DATADIR "/mod_files/test_alternating_reaction.mod"), "test_alternating_reaction.mod"); + EXPECT_NE(m.buffer().size(), 0u); + + Parser p(m, false); + EXPECT_TRUE(p.parse()); + + EXPECT_FALSE(m.semantic()); +} + +TEST(Module, kinetic_footgun_alternating_diff) { + Module m(io::read_all(DATADIR "/mod_files/test_alternating_differential.mod"), "test_alternating_differential.mod"); + EXPECT_NE(m.buffer().size(), 0u); + + Parser p(m, false); + EXPECT_TRUE(p.parse()); + + EXPECT_FALSE(m.semantic()); +} + +TEST(Module, derivative_alternating) { + Module m(io::read_all(DATADIR "/mod_files/test_alternating_derivative.mod"), "test_alternating_derivative.mod"); + EXPECT_NE(m.buffer().size(), 0u); + + Parser p(m, false); + EXPECT_TRUE(p.parse()); + + EXPECT_TRUE(m.semantic()); +} + +TEST(Module, misplaced_reaction) { + Module m(io::read_all(DATADIR "/mod_files/test_misplaced_reaction.mod"), "test_misplaced_reaction.mod"); + EXPECT_NE(m.buffer().size(), 0u); + + Parser p(m, false); + EXPECT_TRUE(p.parse()); + + EXPECT_FALSE(m.semantic()); +} + +TEST(Module, reaction_under_conditional) { + Module m(io::read_all(DATADIR "/mod_files/test_kinetic_under_conditional.mod"), "test_kinetic_under_conditional.mod"); + EXPECT_NE(m.buffer().size(), 0u); + + Parser p(m, false); + EXPECT_FALSE(p.parse()); + + EXPECT_FALSE(m.semantic()); +} From b0993917b7a054e4c44f98073a61161ad2d6d2d4 Mon Sep 17 00:00:00 2001 From: Thorsten Hater <24411438+thorstenhater@users.noreply.github.com> Date: Tue, 19 Sep 2023 10:20:07 +0200 Subject: [PATCH 3/3] Rethrow a more informative error if cell kind is not a `cell_kind` (#2219) Fixes #2138. --- python/recipe.cpp | 3 ++- python/recipe.hpp | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/python/recipe.cpp b/python/recipe.cpp index 16f78494f8..2649ba6bdf 100644 --- a/python/recipe.cpp +++ b/python/recipe.cpp @@ -188,7 +188,8 @@ void register_recipe(pybind11::module& m) { .def("cell_description", &py_recipe::cell_description, pybind11::return_value_policy::copy, "gid"_a, "High level description of the cell with global identifier gid.") - .def("cell_kind", &py_recipe::cell_kind, + .def("cell_kind", + &py_recipe::cell_kind, "gid"_a, "The kind of cell with global identifier gid.") .def("event_generators", &py_recipe::event_generators, diff --git a/python/recipe.hpp b/python/recipe.hpp index fe7624138a..8462b88138 100644 --- a/python/recipe.hpp +++ b/python/recipe.hpp @@ -64,7 +64,12 @@ class py_recipe_trampoline: public py_recipe { } arb::cell_kind cell_kind(arb::cell_gid_type gid) const override { - PYBIND11_OVERRIDE_PURE(arb::cell_kind, py_recipe, cell_kind, gid); + try { + PYBIND11_OVERRIDE_PURE(arb::cell_kind, py_recipe, cell_kind, gid); + } + catch (const pybind11::cast_error& e) { + throw pybind11::type_error{"Couldn't convert return value of recipe.cell_kind(gid) to a cell_kind. Please check your recipe."}; + } } std::vector event_generators(arb::cell_gid_type gid) const override {