Merge branch 'master' into tests_for_particle_diffusion

.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
+    - requirements: doc/requirements.txt

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:
 

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

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<std::string, expression_ptr> dterms;
 };

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,

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<pybind11::object> event_generators(arb::cell_gid_type gid) const override {

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
+}

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)
+}

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)
+}

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)
+  }
+}

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)
+}

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());
+}