Fix deleteSection and appendSection (#251)

* Remove the shared pointer const refs from the methods which change the shared pointer ref count. We had some problems when the const ref is used and added to a vector and then the vector is resized. * Fix the deleteSection method. Problems with the behavior of the c++ map [] accessor when the key is missing (for the rootSection).
BlueBrain · Mar 12, 2021 · 75d944c · 75d944c
1 parent 78de825
commit 75d944c
Show file tree

Hide file tree

Showing 11 changed files with 70 additions and 17 deletions.
diff --git a/binds/python/bind_mutable.cpp b/binds/python/bind_mutable.cpp
@@ -435,7 +435,7 @@ void bind_mutable_module(py::module& m) {
 
         .def("append_section",
              static_cast<std::shared_ptr<morphio::mut::Section> (
-                 morphio::mut::Section::*)(const std::shared_ptr<morphio::mut::Section>&, bool)>(
+                 morphio::mut::Section::*)(std::shared_ptr<morphio::mut::Section>, bool)>(
                  &morphio::mut::Section::appendSection),
              "Append the existing mutable Section to this section\n"
              "If recursive == true, all descendent will be appended as well",

diff --git a/include/morphio/mut/morphology.h b/include/morphio/mut/morphology.h
@@ -145,7 +145,7 @@ class Morphology
        If recursive == true, all descendent sections will be deleted as well
        Else, children will be re-attached to their grand-parent
     **/
-    void deleteSection(const std::shared_ptr<Section>& section, bool recursive = true);
+    void deleteSection(std::shared_ptr<Section> section, bool recursive = true);
 
     /**
        Append the existing morphio::Section as a root section

diff --git a/include/morphio/mut/section.h b/include/morphio/mut/section.h
@@ -93,7 +93,7 @@ class Section: public std::enable_shared_from_this<Section>
 
     std::shared_ptr<Section> appendSection(const morphio::Section&, bool recursive = false);
 
-    std::shared_ptr<Section> appendSection(const std::shared_ptr<Section>& original_section,
+    std::shared_ptr<Section> appendSection(std::shared_ptr<Section> original_section,
                                            bool recursive = false);
 
     std::shared_ptr<Section> appendSection(

diff --git a/src/mut/morphology.cpp b/src/mut/morphology.cpp
@@ -110,7 +110,6 @@ std::shared_ptr<Section> Morphology::appendRootSection(const std::shared_ptr<Sec
     const std::shared_ptr<Section> section_copy(new Section(this, _counter, *section_));
     _register(section_copy);
     _rootSections.push_back(section_copy);
-
     const bool emptySection = section_copy->points().empty();
     if (emptySection)
         printError(Warning::APPENDING_EMPTY_SECTION,
@@ -159,9 +158,7 @@ static void eraseByValue(std::vector<std::shared_ptr<Section>>& vec,
     vec.erase(std::remove(vec.begin(), vec.end(), section), vec.end());
 }
 
-void Morphology::deleteSection(const std::shared_ptr<Section>& section_, bool recursive)
-
-{
+void Morphology::deleteSection(std::shared_ptr<Section> section_, bool recursive) {
     if (!section_)
         return;
     unsigned int id = section_->id();
@@ -176,14 +173,17 @@ void Morphology::deleteSection(const std::shared_ptr<Section>& section_, bool re
             deleteSection(*it, false);
         }
     } else {
-        for (auto& child : section_->children()) {
-            // Re-link children to their "grand-parent"
-            _parent[child->id()] = _parent[id];
-            _children[_parent[id]].push_back(child);
-            if (section_->isRoot())
+        // Careful not to use a reference here or you will face reference invalidation problem
+        // with vector resize
+        for (auto child : section_->children()) {
+            if (section_->isRoot()) {
                 _rootSections.push_back(child);
+            } else {
+                // Re-link children to their "grand-parent"
+                _children[_parent[id]].push_back(child);
+                _parent[child->id()] = _parent[id];
+            }
         }
-
         eraseByValue(_rootSections, section_);
         eraseByValue(_children[_parent[id]], section_);
         _children.erase(id);

diff --git a/src/mut/section.cpp b/src/mut/section.cpp
@@ -89,7 +89,7 @@ std::ostream& operator<<(std::ostream& os, const std::shared_ptr<Section>& secti
     return os;
 }
 
-std::shared_ptr<Section> Section::appendSection(const std::shared_ptr<Section>& original_section,
+std::shared_ptr<Section> Section::appendSection(std::shared_ptr<Section> original_section,
                                                 bool recursive) {
     const std::shared_ptr<Section> ptr(
         new Section(_morphology, _morphology->_counter, *original_section));
@@ -112,8 +112,12 @@ std::shared_ptr<Section> Section::appendSection(const std::shared_ptr<Section>&
     _morphology->_parent[childId] = parentId;
     _morphology->_children[parentId].push_back(ptr);
 
+    // Careful not to use a reference here or you will face ref invalidation problem with vector
+    // resize The argument `original_section` of this function could be a reference to the
+    // `_children` array and that reference might be invalidated by this `push_back` (in case
+    // `vector` needs to reallocate the array)
     if (recursive) {
-        for (const auto& child : original_section->children()) {
+        for (auto child : original_section->children()) {
             ptr->appendSection(child, true);
         }
     }
@@ -142,7 +146,7 @@ std::shared_ptr<Section> Section::appendSection(const morphio::Section& section,
     _morphology->_children[parentId].push_back(ptr);
 
     if (recursive) {
-        for (const auto& child : section.children()) {
+        for (auto child : section.children()) {
             ptr->appendSection(child, true);
         }
     }

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -1,6 +1,7 @@
 set(TESTS_SRC
     main.cpp
     test_morphology.cpp
+    test_mut_morphology.cpp
 )
 
 add_executable(unittests ${TESTS_SRC})

diff --git a/tests/data/single_point_root.asc b/tests/data/single_point_root.asc
@@ -0,0 +1,8 @@
+((Dendrite)
+ (3 -4 0 2)
+ (
+   (3 -10 2 4)
+   |
+   (3 -10 0 2)
+ )
+)
diff --git a/tests/test_1_swc.py b/tests/test_1_swc.py
@@ -8,7 +8,7 @@
 from morphio import (Morphology, RawDataError, SectionType, SomaError, SomaType,
                      ostream_redirect, set_maximum_warnings, set_ignored_warning, Warning)
 from . utils import (_test_swc_exception, assert_substring, assert_string_equal, captured_output,
-                   strip_color_codes, tmp_swc_file, strip_all, ignored_warning)
+                   strip_color_codes, tmp_swc_file, ignored_warning)
 
 _path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "data")
 
@@ -448,6 +448,7 @@ def test_three_point_soma():
     n = Morphology(os.path.join(_path, 'three_point_soma.swc'))
     assert_equal(n.soma_type, SomaType.SOMA_NEUROMORPHO_THREE_POINT_CYLINDERS)
 
+
 def test_version():
     assert_array_equal(Morphology(os.path.join(_path, 'simple.swc')).version,
                        ('swc', 1, 0))
diff --git a/tests/test_5_mut.py b/tests/test_5_mut.py
@@ -467,6 +467,27 @@ def test_sanitize():
                          'Warning: while appending section: 2 to parent: 0\nThe section first point should be parent section last point: \n        : X Y Z Diameter\nparent last point :[2.000000, 0.000000, 0.000000, 2.000000]\nchild first point :[2.000000, 1.000000, 0.000000, 2.000000]')
 
 
+def test_remove_rootsection():
+    morpho = Morphology(os.path.join(_path, 'single_point_root.asc'))
+    assert_equal(len(morpho.root_sections), 1)
+    to_remove = []
+    for root in morpho.root_sections:
+        if len(root.points) == 1:
+            to_remove.append(root)
+    for root in to_remove:
+        morpho.delete_section(root, False)
+    assert_equal(len(morpho.root_sections), 2)
+
+
+def test_remove_rootsection_in_loop():
+    morpho = Morphology(os.path.join(_path, 'single_point_root.asc'))
+    assert_equal(len(morpho.root_sections), 1)
+    for root in morpho.root_sections:
+        if len(root.points) == 1:
+            morpho.delete_section(root, False)
+    assert_equal(len(morpho.root_sections), 2)
+
+
 def test_glia():
     g = GlialCell()
     assert_equal(g.cell_family, CellFamily.GLIA)

diff --git a/tests/test_morphology.cpp b/tests/test_morphology.cpp
@@ -3,6 +3,7 @@
 
 #include <highfive/H5File.hpp>
 #include <morphio/morphology.h>
+#include <morphio/mut/morphology.h>
 
 
 TEST_CASE("LoadH5Morphology", "[morphology]") {

diff --git a/tests/test_mut_morphology.cpp b/tests/test_mut_morphology.cpp
@@ -0,0 +1,17 @@
+#include "contrib/catch.hpp"
+
+#include <morphio/mut/morphology.h>
+
+
+TEST_CASE("RemoveRootsection", "[mutMorphology]") {
+    // this test verifies we can delete a root section with recursive at false from a morphology
+    // correctly. This is a special case where the root section as a single point and 2 child
+    // sections. The number of sections is small enough to trigger a resize on _rootSections.
+    morphio::mut::Morphology morpho("data/single_point_root.asc");
+    for (const auto& rootSection : morpho.rootSections()) {
+        if (rootSection->points().size() == 1) {
+            morpho.deleteSection(rootSection, false);
+        }
+    }
+    REQUIRE(morpho.rootSections().size() == 2);
+}