From cfe5f4bf2be8915ea3d1296e30c8e1655b3315d8 Mon Sep 17 00:00:00 2001
From: Sam Yates <halfflat@gmail.com>
Date: Tue, 6 Sep 2016 11:50:05 +0200
Subject: [PATCH] Address PR#92 review comments.

Note: use of non-const reference in `mechanism_catalogue::make`
is an interim solution.
---
 src/cell_group.hpp                            |  6 +--
 src/communication/communicator.hpp            |  2 +-
 src/fvm_multicell.hpp                         | 49 ++++++++++---------
 src/mechanism.hpp                             | 11 ++---
 src/mechanism_catalogue.hpp                   |  5 +-
 src/util/partition.hpp                        |  2 +
 .../test_mpi_gather_all.cpp                   |  2 +-
 7 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/src/cell_group.hpp b/src/cell_group.hpp
index 30bdd99d..86a90593 100644
--- a/src/cell_group.hpp
+++ b/src/cell_group.hpp
@@ -249,13 +249,13 @@ private:
 
     /// use handle partition to get index from id
     template <typename Divisions>
-    std::size_t handle_partition_lookup(const Divisions& divisions_, cell_member_type id) const {
+    std::size_t handle_partition_lookup(const Divisions& divisions, cell_member_type id) const {
         // NB: without any assertion checking, this would just be:
-        // return divisions_[id.gid-gid_base_]+id.index;
+        // return divisions[id.gid-gid_base_]+id.index;
 
         EXPECTS(id.gid>=gid_base_);
 
-        auto handle_partition = util::partition_view(divisions_);
+        auto handle_partition = util::partition_view(divisions);
         EXPECTS(id.gid-gid_base_<handle_partition.size());
 
         auto ival = handle_partition[id.gid-gid_base_];
diff --git a/src/communication/communicator.hpp b/src/communication/communicator.hpp
index 6d1377f4..764020dc 100644
--- a/src/communication/communicator.hpp
+++ b/src/communication/communicator.hpp
@@ -136,7 +136,7 @@ public:
 private:
     std::size_t cell_group_index(cell_gid_type cell_gid) const {
         EXPECTS(is_local_cell(cell_gid));
-        return cell_gid_partition_.find(cell_gid)-cell_gid_partition_.begin();
+        return cell_gid_partition_.index(cell_gid);
     }
 
     std::vector<connection_type> connections_;
diff --git a/src/fvm_multicell.hpp b/src/fvm_multicell.hpp
index 1c9c799a..72d22b25 100644
--- a/src/fvm_multicell.hpp
+++ b/src/fvm_multicell.hpp
@@ -214,7 +214,7 @@ private:
 
     // perform area and capacitance calculation on initialization
     void compute_cv_area_unnormalized_capacitance(
-        std::pair<size_type, size_type> comps, const segment* seg, index_type &parent);
+        std::pair<size_type, size_type> comp_ival, const segment* seg, index_type &parent);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -223,24 +223,25 @@ private:
 
 template <typename T, typename I>
 void fvm_multicell<T, I>::compute_cv_area_unnormalized_capacitance(
-    std::pair<size_type, size_type> comps,
+    std::pair<size_type, size_type> comp_ival,
     const segment* seg,
     index_type &parent)
 {
     using util::left;
     using util::right;
+    using util::size;
 
     // precondition: group_parent_index[j] holds the correct value for
     // j in [base_comp, base_comp+segment.num_compartments()].
 
-    auto ncomp = comps.second-comps.first;
+    auto ncomp = comp_ival.second-comp_ival.first;
 
     if (auto soma = seg->as_soma()) {
         // confirm assumption that there is one compartment in soma
         if (ncomp!=1) {
             throw std::logic_error("soma allocated more than one compartment");
         }
-        auto i = comps.first;
+        auto i = comp_ival.first;
         auto area = math::area_sphere(soma->radius());
 
         cv_areas_[i] += area;
@@ -266,10 +267,10 @@ void fvm_multicell<T, I>::compute_cv_area_unnormalized_capacitance(
         auto r_L = cable->mechanism("membrane").get("r_L").value;
         const auto& compartments = cable->compartments();
 
-        EXPECTS(util::size(compartments)==ncomp);
+        EXPECTS(size(compartments)==ncomp);
 
-        for (auto i: util::make_span(comps)) {
-            const auto& c = compartments[i-comps.first];
+        for (auto i: util::make_span(comp_ival)) {
+            const auto& c = compartments[i-comp_ival.first];
             auto j = parent[i];
 
             auto radius_center = math::mean(c.radius);
@@ -308,11 +309,11 @@ void fvm_multicell<T, I>::initialize(
     size_type detectors_count = 0u;
     size_type targets_count = 0u;
     size_type probes_count = 0u;
-    size_type detectors_size = util::size(detector_handles);
-    size_type targets_size = util::size(target_handles);
-    size_type probes_size = util::size(probe_handles);
+    size_type detectors_size = size(detector_handles);
+    size_type targets_size = size(target_handles);
+    size_type probes_size = size(probe_handles);
 
-    auto ncell = util::size(cells);
+    auto ncell = size(cells);
     auto cell_num_compartments =
         transform_view(cells, [](const cell& c) { return c.num_compartments(); });
 
@@ -325,7 +326,7 @@ void fvm_multicell<T, I>::initialize(
     face_alpha_ = vector_type{ncomp, T{0}};
     cv_capacitance_ = vector_type{ncomp, T{0}};
     current_    = vector_type{ncomp, T{0}};
-    voltage_    = vector_type{ncomp, T{0}};
+    voltage_    = vector_type{ncomp, T{resting_potential_}};
 
     // create maps for mechanism initialization.
     std::map<std::string, std::vector<std::pair<size_type, size_type>>> mech_map;
@@ -342,12 +343,12 @@ void fvm_multicell<T, I>::initialize(
 
     for (size_type i = 0; i<ncell; ++i) {
         const auto& c = cells[i];
-        auto comps = cell_comp_part[i];
+        auto comp_ival = cell_comp_part[i];
 
         auto graph = c.model();
 
-        for (auto k: make_span(comps)) {
-            group_parent_index[k] = graph.parent_index[k-comps.first]+comps.first;
+        for (auto k: make_span(comp_ival)) {
+            group_parent_index[k] = graph.parent_index[k-comp_ival.first]+comp_ival.first;
         }
 
         auto seg_num_compartments =
@@ -355,17 +356,17 @@ void fvm_multicell<T, I>::initialize(
         auto nseg = seg_num_compartments.size();
 
         std::vector<cell_lid_type> seg_comp_bounds;
-        auto seg_comp_part = make_partition(seg_comp_bounds, seg_num_compartments, comps.first);
+        auto seg_comp_part = make_partition(seg_comp_bounds, seg_num_compartments, comp_ival.first);
 
         for (size_type j = 0; j<nseg; ++j) {
             const auto& seg = c.segment(j);
-            const auto& seg_comps = seg_comp_part[j];
+            const auto& seg_comp_ival = seg_comp_part[j];
 
-            compute_cv_area_unnormalized_capacitance(seg_comps, seg, group_parent_index);
+            compute_cv_area_unnormalized_capacitance(seg_comp_ival, seg, group_parent_index);
 
             for (const auto& mech: seg->mechanisms()) {
                 if (mech.name()!="membrane") {
-                    mech_map[mech.name()].push_back(seg_comps);
+                    mech_map[mech.name()].push_back(seg_comp_ival);
                 }
             }
         }
@@ -386,7 +387,7 @@ void fvm_multicell<T, I>::initialize(
 
             auto& map_entry = syn_mech_map[syn_mech_index];
 
-            size_type syn_comp = comps.first+find_compartment_index(syn.location, graph);
+            size_type syn_comp = comp_ival.first+find_compartment_index(syn.location, graph);
             size_type syn_index = map_entry.size();
             map_entry.push_back(syn_comp);
 
@@ -395,13 +396,13 @@ void fvm_multicell<T, I>::initialize(
         }
 
         // normalize capacitance across cell
-        for (auto k: make_span(comps)) {
+        for (auto k: make_span(comp_ival)) {
             cv_capacitance_[k] /= cv_areas_[k];
         }
 
         // add the stimuli
         for (const auto& stim: c.stimuli()) {
-            auto idx = comps.first+find_compartment_index(stim.location, graph);
+            auto idx = comp_ival.first+find_compartment_index(stim.location, graph);
             stimuli_.push_back({idx, stim.clamp});
         }
 
@@ -409,7 +410,7 @@ void fvm_multicell<T, I>::initialize(
         for (const auto& detector: c.detectors()) {
             EXPECTS(detectors_count < detectors_size);
 
-            auto comp = comps.first+find_compartment_index(detector.location, graph);
+            auto comp = comp_ival.first+find_compartment_index(detector.location, graph);
             *detector_hi++ = comp;
             ++detectors_count;
         }
@@ -418,7 +419,7 @@ void fvm_multicell<T, I>::initialize(
         for (const auto& probe: c.probes()) {
             EXPECTS(probes_count < probes_size);
 
-            auto comp = comps.first+find_compartment_index(probe.location, graph);
+            auto comp = comp_ival.first+find_compartment_index(probe.location, graph);
             switch (probe.kind) {
             case probeKind::membrane_voltage:
                 *probe_hi++ = {&fvm_multicell::voltage_, comp};
diff --git a/src/mechanism.hpp b/src/mechanism.hpp
index c9c801fd..ce978660 100644
--- a/src/mechanism.hpp
+++ b/src/mechanism.hpp
@@ -1,7 +1,9 @@
 #pragma once
 
+#include <algorithm>
 #include <memory>
 #include <string>
+#include <util/meta.hpp>
 
 #include "indexed_view.hpp"
 #include "ion.hpp"
@@ -16,9 +18,7 @@ enum class mechanismKind {point, density};
 
 template <typename T, typename I>
 class mechanism {
-
 public:
-
     using value_type  = T;
     using size_type   = I;
 
@@ -31,11 +31,8 @@ public:
 
     using ion_type    = ion<value_type, size_type>;
 
-    mechanism(view_type vec_v, view_type vec_i, index_view node_index)
-    :   vec_v_(vec_v)
-    ,   vec_i_(vec_i)
-    ,   node_index_(node_index)
-    ,   vec_area_(nullptr, 0)
+    mechanism(view_type vec_v, view_type vec_i, index_view node_index):
+        vec_v_(vec_v), vec_i_(vec_i), node_index_(node_index), vec_area_(nullptr, 0)
     {}
 
     std::size_t size() const
diff --git a/src/mechanism_catalogue.hpp b/src/mechanism_catalogue.hpp
index aecea5ce..7fb2927c 100644
--- a/src/mechanism_catalogue.hpp
+++ b/src/mechanism_catalogue.hpp
@@ -24,14 +24,15 @@ struct catalogue {
         const std::string& name,
         view_type vec_v,
         view_type vec_i,
-        Indices node_indices)
+        Indices& node_indices)
     {
         auto entry = mech_map.find(name);
         if (entry==mech_map.end()) {
             throw std::out_of_range("no such mechanism");
         }
 
-        return entry->second(vec_v, vec_i, index_view{node_indices});
+        auto node_view = index_view{node_indices};
+        return entry->second(vec_v, vec_i, node_view);
     }
 
     static bool has(const std::string& name) {
diff --git a/src/util/partition.hpp b/src/util/partition.hpp
index 376b769e..425d41f1 100644
--- a/src/util/partition.hpp
+++ b/src/util/partition.hpp
@@ -36,6 +36,8 @@ public:
     using base::back;
     using base::empty;
 
+    // `npos` is returned by the `index()` method if the search fails;
+    // analogous to `std::string::npos`.
     static constexpr size_type npos = static_cast<size_type>(-1);
 
     partition_range() = default;
diff --git a/tests/global_communication/test_mpi_gather_all.cpp b/tests/global_communication/test_mpi_gather_all.cpp
index ed99089f..50fbf405 100644
--- a/tests/global_communication/test_mpi_gather_all.cpp
+++ b/tests/global_communication/test_mpi_gather_all.cpp
@@ -17,7 +17,7 @@ struct big_thing {
     big_thing(int i): value_(i) {}
 
     bool operator==(const big_thing& other) const {
-        return value_==other.value_ && !memcmp(salt_, other.salt_, sizeof(salt_));
+        return value_==other.value_ && !std::memcmp(salt_, other.salt_, sizeof(salt_));
     }
 
     bool operator!=(const big_thing& other) const {
-- 
GitLab