From 616d80c1208f3a14b871b0105f56ee9abd98a82c Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Tue, 21 Aug 2018 08:06:31 +0000 Subject: Copy assignment for Simplex_tree and its tests (rules of 5) git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3814 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 4ac0e87aaef3bf29369f25b8eb0c766a6f1b395b --- src/Simplex_tree/test/CMakeLists.txt | 8 +++ .../test/simplex_tree_ctor_and_move_unit_test.cpp | 63 ++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/test/CMakeLists.txt b/src/Simplex_tree/test/CMakeLists.txt index c63d8532..5bea3938 100644 --- a/src/Simplex_tree/test/CMakeLists.txt +++ b/src/Simplex_tree/test/CMakeLists.txt @@ -28,3 +28,11 @@ if (TBB_FOUND) endif() gudhi_add_coverage_test(Simplex_tree_iostream_operator_test_unit) + +add_executable ( Simplex_tree_ctor_and_move_test_unit simplex_tree_ctor_and_move_unit_test.cpp ) +target_link_libraries(Simplex_tree_ctor_and_move_test_unit ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY}) +if (TBB_FOUND) + target_link_libraries(Simplex_tree_ctor_and_move_test_unit ${TBB_LIBRARIES}) +endif() + +gudhi_add_coverage_test(Simplex_tree_ctor_and_move_test_unit) diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp new file mode 100644 index 00000000..c9439e65 --- /dev/null +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -0,0 +1,63 @@ +#include +#include +#include +#include +#include // std::pair, std::make_pair +#include // float comparison +#include +#include // greater + +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_MODULE "simplex_tree_constructor_and_move" +#include +#include + +// ^ +// /!\ Nothing else from Simplex_tree shall be included to test includes are well defined. +#include "gudhi/Simplex_tree.h" + +using namespace Gudhi; + +typedef boost::mpl::list, Simplex_tree> list_of_tested_variants; + + +BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_tested_variants) { + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF COPY CONSTRUCTOR" << std::endl; + Simplex_tree st; + + st.insert_simplex_and_subfaces({2, 1, 0}, 3.0); + st.insert_simplex_and_subfaces({0, 1, 6, 7}, 4.0); + st.insert_simplex_and_subfaces({3, 0}, 2.0); + st.insert_simplex_and_subfaces({3, 4, 5}, 3.0); + /* Inserted simplex: */ + /* 1 6 */ + /* o---o */ + /* /X\7/ */ + /* o---o---o---o */ + /* 2 0 3\X/4 */ + /* o */ + /* 5 */ + /* */ + /* In other words: */ + /* A facet [2,1,0] */ + /* An edge [0,3] */ + /* A facet [3,4,5] */ + /* A cell [0,1,6,7] */ + + Simplex_tree st1(st); + Simplex_tree st2(st); + // Cross check + BOOST_CHECK(st1 == st2); + BOOST_CHECK(st == st2); + BOOST_CHECK(st1 == st); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF COPY ASSIGNMENT" << std::endl; + Simplex_tree st3 = st; + Simplex_tree st4 = st; + // Cross check + BOOST_CHECK(st3 == st4); + BOOST_CHECK(st == st4); + BOOST_CHECK(st3 == st); +} -- cgit v1.2.3 From b83a2c100cf19eb3cd6ddf4fb0dca9ddcec52906 Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Tue, 21 Aug 2018 08:44:32 +0000 Subject: Bad copy assignment test git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3815 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: c5c9cd3c0553b3367452489e7fd5e874f5093742 --- src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index c9439e65..b963d4ee 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -54,8 +54,10 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF COPY ASSIGNMENT" << std::endl; - Simplex_tree st3 = st; - Simplex_tree st4 = st; + Simplex_tree st3; + st3 = st; + Simplex_tree st4; + st4 = st; // Cross check BOOST_CHECK(st3 == st4); BOOST_CHECK(st == st4); -- cgit v1.2.3 From 23c6f4e26b60374d9df37445598c087ff6b52512 Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Tue, 21 Aug 2018 12:05:28 +0000 Subject: Add DEBUG_TRACES for the test Add move constructor test git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3818 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 4e9baaedcbcb6fdfc7b5ee0ac42bc75559b67bd1 --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 6 ++++++ .../test/simplex_tree_ctor_and_move_unit_test.cpp | 11 +++++++++++ 2 files changed, 17 insertions(+) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index ca3575ba..d604f994 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -301,7 +301,9 @@ class Simplex_tree { root_(nullptr, null_vertex_ , simplex_source.root_.members_), filtration_vect_(), dimension_(simplex_source.dimension_) { +#ifdef DEBUG_TRACES std::cout << "copy constructor" << std::endl; +#endif // DEBUG_TRACES auto root_source = simplex_source.root_; rec_copy(&root_, &root_source); } @@ -327,7 +329,9 @@ class Simplex_tree { root_(std::move(old.root_)), filtration_vect_(std::move(old.filtration_vect_)), dimension_(std::move(old.dimension_)) { +#ifdef DEBUG_TRACES std::cout << "move constructor" << std::endl; +#endif // DEBUG_TRACES old.dimension_ = -1; old.root_ = Siblings(nullptr, null_vertex_); } @@ -344,7 +348,9 @@ class Simplex_tree { /** \brief User-defined copy assignment reproduces the whole tree structure. */ Simplex_tree& operator= (const Simplex_tree& simplex_source) { +#ifdef DEBUG_TRACES std::cout << "copy assignment" << std::endl; +#endif // DEBUG_TRACES this->null_vertex_ = simplex_source.null_vertex_; root_ = Siblings(nullptr, null_vertex_ , simplex_source.root_.members_); this->filtration_vect_.clear(); diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index b963d4ee..fb3e595c 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -62,4 +62,15 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te BOOST_CHECK(st3 == st4); BOOST_CHECK(st == st4); BOOST_CHECK(st3 == st); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF MOVE CONSTRUCTOR" << std::endl; + Simplex_tree st5(std::move(st3)); + Simplex_tree st6(std::move(st4)); + + // Cross check + BOOST_CHECK(st5 == st6); + BOOST_CHECK(st == st6); + BOOST_CHECK(st5 == st); + } -- cgit v1.2.3 From ad82d011d06c22ce77817ab4606bd0e15663a145 Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Tue, 21 Aug 2018 13:03:33 +0000 Subject: Move assignment and its associated test git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3819 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 6fd6793b177e0a6b803adf5a710f63d8647a9288 --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 15 +++++++++++++ .../test/simplex_tree_ctor_and_move_unit_test.cpp | 26 ++++++++++++++-------- 2 files changed, 32 insertions(+), 9 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index d604f994..4759b352 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -360,6 +360,21 @@ class Simplex_tree { return *this; } + /** \brief User-defined move assignment reproduces the whole tree structure. */ + Simplex_tree& operator=(Simplex_tree&& simplex_source) + { +#ifdef DEBUG_TRACES + std::cout << "move assignment" << std::endl; +#endif // DEBUG_TRACES + // Self-assignment detection + if (&simplex_source != this) { + std::swap( null_vertex_, simplex_source.null_vertex_ ); + std::swap( root_, simplex_source.root_ ); + std::swap( filtration_vect_, simplex_source.filtration_vect_ ); + std::swap( dimension_, simplex_source.dimension_ ); + } + return *this; + } /** @} */ // end constructor/destructor private: // Recursive deletion diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index fb3e595c..15eaf612 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -1,11 +1,5 @@ #include -#include -#include -#include -#include // std::pair, std::make_pair -#include // float comparison -#include -#include // greater +#include #define BOOST_TEST_DYN_LINK #define BOOST_TEST_MODULE "simplex_tree_constructor_and_move" @@ -20,6 +14,11 @@ using namespace Gudhi; typedef boost::mpl::list, Simplex_tree> list_of_tested_variants; +template +SimplicialComplex move_it(SimplicialComplex sc) { + return sc; +} + BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_tested_variants) { std::cout << "********************************************************************" << std::endl; @@ -65,12 +64,21 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE CONSTRUCTOR" << std::endl; - Simplex_tree st5(std::move(st3)); - Simplex_tree st6(std::move(st4)); + Simplex_tree st5(std::move(st1)); + Simplex_tree st6(std::move(st2)); // Cross check BOOST_CHECK(st5 == st6); BOOST_CHECK(st == st6); BOOST_CHECK(st5 == st); + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF MOVE ASSIGNMENT" << std::endl; + + // A swap is a copy ctor of a tmp value, then it uses move assignment + std::swap(st3, st4); + BOOST_CHECK(st3 == st4); + BOOST_CHECK(st == st4); + BOOST_CHECK(st3 == st); + } -- cgit v1.2.3 From af065fc6d2d4771485118bb5122b3d5203b367ed Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Fri, 24 Aug 2018 07:39:49 +0000 Subject: Fix move constructor and assignment for the Simplex tree git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3831 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: ff520472b6a1b9f00d0688fbe7dc467af50e16fe --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 64 ++++++++++++++++------ .../test/simplex_tree_ctor_and_move_unit_test.cpp | 59 ++++++++++++++++---- 2 files changed, 94 insertions(+), 29 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 4759b352..8e0ddf75 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -298,11 +298,11 @@ class Simplex_tree { /** \brief User-defined copy constructor reproduces the whole tree structure. */ Simplex_tree(const Simplex_tree& simplex_source) : null_vertex_(simplex_source.null_vertex_), - root_(nullptr, null_vertex_ , simplex_source.root_.members_), + root_(nullptr, null_vertex_, simplex_source.root_.members_), filtration_vect_(), dimension_(simplex_source.dimension_) { #ifdef DEBUG_TRACES - std::cout << "copy constructor" << std::endl; + std::cout << "Simplex_tree copy constructor" << std::endl; #endif // DEBUG_TRACES auto root_source = simplex_source.root_; rec_copy(&root_, &root_source); @@ -323,17 +323,29 @@ class Simplex_tree { } } - /** \brief User-defined move constructor moves the whole tree structure. */ - Simplex_tree(Simplex_tree && old) - : null_vertex_(std::move(old.null_vertex_)), - root_(std::move(old.root_)), - filtration_vect_(std::move(old.filtration_vect_)), - dimension_(std::move(old.dimension_)) { + /** \brief User-defined move constructor relocates the whole tree structure. + * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + */ + Simplex_tree(Simplex_tree && simplex_source) + : null_vertex_(std::move(simplex_source.null_vertex_)), + root_(std::move(simplex_source.root_)), + filtration_vect_(std::move(simplex_source.filtration_vect_)), + dimension_(std::move(simplex_source.dimension_)) { #ifdef DEBUG_TRACES - std::cout << "move constructor" << std::endl; + std::cout << "Simplex_tree move constructor" << std::endl; #endif // DEBUG_TRACES - old.dimension_ = -1; - old.root_ = Siblings(nullptr, null_vertex_); + for (auto& map_el : root_.members()) { + if (map_el.second.children()->oncles() == &(simplex_source.root_)) + // reset with the moved root_ pointer value + map_el.second.children()->oncles_ = &root_; + else + // if simplex is of dimension 0, oncles_ shall be nullptr + GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, + std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); + } + // just need to set dimension_ on source to make it available again + // (filtration_vect_ and members are already set from the move) + simplex_source.dimension_ = -1; } /** \brief Destructor; deallocates the whole tree structure. */ @@ -351,16 +363,24 @@ class Simplex_tree { #ifdef DEBUG_TRACES std::cout << "copy assignment" << std::endl; #endif // DEBUG_TRACES - this->null_vertex_ = simplex_source.null_vertex_; - root_ = Siblings(nullptr, null_vertex_ , simplex_source.root_.members_); - this->filtration_vect_.clear(); - this->dimension_ = simplex_source.dimension_; + null_vertex_ = simplex_source.null_vertex_; + filtration_vect_.clear(); + dimension_ = simplex_source.dimension_; auto root_source = simplex_source.root_; - rec_copy(&(this->root_), &root_source); + // Here a copy will be done + root_ = Siblings(nullptr, null_vertex_); + root_.members() = Dictionary(boost::container::ordered_unique_range, root_source.members().begin(), root_source.members().end()); + // Needs to reassign children + for (auto& map_el : root_.members()) { + map_el.second.assign_children(&root_); + } + rec_copy(&root_, &root_source); return *this; } - /** \brief User-defined move assignment reproduces the whole tree structure. */ + /** \brief User-defined move assignment relocates the whole tree structure. + * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + */ Simplex_tree& operator=(Simplex_tree&& simplex_source) { #ifdef DEBUG_TRACES @@ -372,6 +392,16 @@ class Simplex_tree { std::swap( root_, simplex_source.root_ ); std::swap( filtration_vect_, simplex_source.filtration_vect_ ); std::swap( dimension_, simplex_source.dimension_ ); + + for (auto& map_el : root_.members()) { + if (map_el.second.children()->oncles() == &(simplex_source.root_)) + // reset with the moved root_ pointer value + map_el.second.children()->oncles_ = &root_; + else + // if simplex is of dimension 0, oncles_ shall be nullptr + GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, + std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); + } } return *this; } diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index 15eaf612..95e99afe 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -1,5 +1,6 @@ #include #include +#include #define BOOST_TEST_DYN_LINK #define BOOST_TEST_MODULE "simplex_tree_constructor_and_move" @@ -14,38 +15,58 @@ using namespace Gudhi; typedef boost::mpl::list, Simplex_tree> list_of_tested_variants; -template -SimplicialComplex move_it(SimplicialComplex sc) { - return sc; -} +template +void print_simplex_filtration(Simplex_tree& st, const std::string& msg) { + // Required before browsing through filtration values + st.initialize_filtration(); + + std::cout << "********************************************************************\n"; + std::cout << "* " << msg << "\n"; + std::cout << "* The complex contains " << st.num_simplices() << " simplices"; + std::cout << " - dimension " << st.dimension() << "\n"; + std::cout << "* Iterator on Simplices in the filtration, with [filtration value]:\n"; + for (auto f_simplex : st.filtration_simplex_range()) { + std::cout << " " + << "[" << st.filtration(f_simplex) << "] "; + for (auto vertex : st.simplex_vertex_range(f_simplex)) std::cout << "(" << vertex << ")"; + std::cout << std::endl; + } +} BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_tested_variants) { - std::cout << "********************************************************************" << std::endl; - std::cout << "TEST OF COPY CONSTRUCTOR" << std::endl; Simplex_tree st; st.insert_simplex_and_subfaces({2, 1, 0}, 3.0); st.insert_simplex_and_subfaces({0, 1, 6, 7}, 4.0); st.insert_simplex_and_subfaces({3, 0}, 2.0); st.insert_simplex_and_subfaces({3, 4, 5}, 3.0); + st.insert_simplex_and_subfaces({8}, 1.0); /* Inserted simplex: */ /* 1 6 */ /* o---o */ /* /X\7/ */ - /* o---o---o---o */ - /* 2 0 3\X/4 */ + /* o---o---o---o o */ + /* 2 0 3\X/4 8 */ /* o */ /* 5 */ /* */ /* In other words: */ - /* A facet [2,1,0] */ - /* An edge [0,3] */ - /* A facet [3,4,5] */ - /* A cell [0,1,6,7] */ + /* A facet [2,1,0] */ + /* An edge [0,3] */ + /* A facet [3,4,5] */ + /* A cell [0,1,6,7] */ + /* A vertex [8] */ + + print_simplex_filtration(st, "Default Simplex_tree is initialized"); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF COPY CONSTRUCTOR" << std::endl; Simplex_tree st1(st); Simplex_tree st2(st); + print_simplex_filtration(st1, "First copy constructor from the default Simplex_tree"); + print_simplex_filtration(st2, "Second copy constructor from the default Simplex_tree"); // Cross check BOOST_CHECK(st1 == st2); BOOST_CHECK(st == st2); @@ -55,8 +76,11 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "TEST OF COPY ASSIGNMENT" << std::endl; Simplex_tree st3; st3 = st; + print_simplex_filtration(st3, "First copy assignment from the default Simplex_tree"); Simplex_tree st4; st4 = st; + print_simplex_filtration(st4, "Second copy assignment from the default Simplex_tree"); + // Cross check BOOST_CHECK(st3 == st4); BOOST_CHECK(st == st4); @@ -65,18 +89,29 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE CONSTRUCTOR" << std::endl; Simplex_tree st5(std::move(st1)); + print_simplex_filtration(st5, "First move constructor from the default Simplex_tree"); + print_simplex_filtration(st1, "First moved Simplex_tree shall be empty"); Simplex_tree st6(std::move(st2)); + print_simplex_filtration(st6, "Second move constructor from the default Simplex_tree"); + print_simplex_filtration(st2, "Second moved Simplex_tree shall be empty"); // Cross check BOOST_CHECK(st5 == st6); BOOST_CHECK(st == st6); BOOST_CHECK(st5 == st); + Simplex_tree empty_st; + BOOST_CHECK(st1 == st2); + BOOST_CHECK(empty_st == st2); + BOOST_CHECK(st1 == empty_st); + std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE ASSIGNMENT" << std::endl; // A swap is a copy ctor of a tmp value, then it uses move assignment std::swap(st3, st4); + print_simplex_filtration(st3, "First move assignment from the default Simplex_tree"); + print_simplex_filtration(st4, "Second move assignment from the default Simplex_tree"); BOOST_CHECK(st3 == st4); BOOST_CHECK(st == st4); BOOST_CHECK(st3 == st); -- cgit v1.2.3 From 527d4871c7826d5a07686c5e3f1f6edb18fbdc3d Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Mon, 3 Sep 2018 15:00:41 +0000 Subject: Code review : Fix remarks for move and copy assignment git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3868 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 069e920ab3ca9504b5e1c49ab23afe1693f4e6f8 --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 93 ++++++++++++---------- .../test/simplex_tree_ctor_and_move_unit_test.cpp | 22 +++-- 2 files changed, 67 insertions(+), 48 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 69ed5e13..20b527a2 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -296,15 +296,15 @@ class Simplex_tree { dimension_(-1) { } /** \brief User-defined copy constructor reproduces the whole tree structure. */ - Simplex_tree(const Simplex_tree& simplex_source) - : null_vertex_(simplex_source.null_vertex_), - root_(nullptr, null_vertex_, simplex_source.root_.members_), + Simplex_tree(const Simplex_tree& complex_source) + : null_vertex_(complex_source.null_vertex_), + root_(nullptr, null_vertex_, complex_source.root_.members_), filtration_vect_(), - dimension_(simplex_source.dimension_) { + dimension_(complex_source.dimension_) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree copy constructor" << std::endl; #endif // DEBUG_TRACES - auto root_source = simplex_source.root_; + auto root_source = complex_source.root_; rec_copy(&root_, &root_source); } @@ -324,34 +324,32 @@ class Simplex_tree { } /** \brief User-defined move constructor relocates the whole tree structure. - * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ - Simplex_tree(Simplex_tree && simplex_source) - : null_vertex_(std::move(simplex_source.null_vertex_)), - root_(std::move(simplex_source.root_)), - filtration_vect_(std::move(simplex_source.filtration_vect_)), - dimension_(std::move(simplex_source.dimension_)) { + Simplex_tree(Simplex_tree && complex_source) + : null_vertex_(std::move(complex_source.null_vertex_)), + root_(std::move(complex_source.root_)), + filtration_vect_(std::move(complex_source.filtration_vect_)), + dimension_(std::move(complex_source.dimension_)) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree move constructor" << std::endl; #endif // DEBUG_TRACES // Need to update root members (children->oncles and children need to point on the new root pointer) for (auto& map_el : root_.members()) { - // children->oncles update after the move - if (map_el.second.children()->oncles() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + if (map_el.second.children()->oncles() == &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value map_el.second.children()->oncles_ = &root_; - else + } else { // if simplex is of dimension 0, oncles_ shall be nullptr GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // children update after the move - if (map_el.second.children() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + // and children points on root_ - to be moved map_el.second.assign_children(&root_); + } } // just need to set dimension_ on source to make it available again // (filtration_vect_ and members are already set from the move) - simplex_source.dimension_ = -1; + complex_source.dimension_ = -1; } /** \brief Destructor; deallocates the whole tree structure. */ @@ -364,17 +362,24 @@ class Simplex_tree { } /** \brief User-defined copy assignment reproduces the whole tree structure. */ - Simplex_tree& operator= (const Simplex_tree& simplex_source) + Simplex_tree& operator= (const Simplex_tree& complex_source) { #ifdef DEBUG_TRACES - std::cout << "copy assignment" << std::endl; + std::cout << "Simplex_tree copy assignment" << std::endl; #endif // DEBUG_TRACES - null_vertex_ = simplex_source.null_vertex_; + null_vertex_ = complex_source.null_vertex_; filtration_vect_.clear(); - dimension_ = simplex_source.dimension_; - auto root_source = simplex_source.root_; - // Here a copy will be done - root_ = Siblings(nullptr, null_vertex_); + dimension_ = complex_source.dimension_; + auto root_source = complex_source.root_; + + // We start by deleting root_ if not empty + for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { + if (has_children(sh)) { + rec_delete(sh->second.children()); + } + } + + // root members copy root_.members() = Dictionary(boost::container::ordered_unique_range, root_source.members().begin(), root_source.members().end()); // Needs to reassign children for (auto& map_el : root_.members()) { @@ -385,35 +390,41 @@ class Simplex_tree { } /** \brief User-defined move assignment relocates the whole tree structure. - * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ - Simplex_tree& operator=(Simplex_tree&& simplex_source) + Simplex_tree& operator=(Simplex_tree&& complex_source) { #ifdef DEBUG_TRACES - std::cout << "move assignment" << std::endl; + std::cout << "Simplex_tree move assignment" << std::endl; #endif // DEBUG_TRACES // Self-assignment detection - if (&simplex_source != this) { - std::swap( null_vertex_, simplex_source.null_vertex_ ); - std::swap( root_, simplex_source.root_ ); - std::swap( filtration_vect_, simplex_source.filtration_vect_ ); - std::swap( dimension_, simplex_source.dimension_ ); + if (&complex_source != this) { + std::swap( null_vertex_, complex_source.null_vertex_ ); + std::swap( root_, complex_source.root_ ); + std::swap( filtration_vect_, complex_source.filtration_vect_ ); + std::swap( dimension_, complex_source.dimension_ ); // Need to update root members (children->oncles and children need to point on the new root pointer) for (auto& map_el : root_.members()) { - // children->oncles update after the move - if (map_el.second.children()->oncles() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + if (map_el.second.children()->oncles() == &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value map_el.second.children()->oncles_ = &root_; - else + } else { // if simplex is of dimension 0, oncles_ shall be nullptr GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // children update after the move - if (map_el.second.children() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + // and children points on root_ - to be moved map_el.second.assign_children(&root_); + } + } + // complex_source root_ deletion + for (auto sh = complex_source.root_.members().begin(); sh != complex_source.root_.members().end(); ++sh) { + if (has_children(sh)) { + rec_delete(sh->second.children()); + } } + complex_source.root_.members().clear(); + complex_source.dimension_ = -1; } return *this; } diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index 95e99afe..6c27a458 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -75,6 +75,8 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF COPY ASSIGNMENT" << std::endl; Simplex_tree st3; + // To check there is no memory leak + st3.insert_simplex_and_subfaces({9, 10, 11}, 200.0); st3 = st; print_simplex_filtration(st3, "First copy assignment from the default Simplex_tree"); Simplex_tree st4; @@ -108,12 +110,18 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE ASSIGNMENT" << std::endl; - // A swap is a copy ctor of a tmp value, then it uses move assignment - std::swap(st3, st4); - print_simplex_filtration(st3, "First move assignment from the default Simplex_tree"); - print_simplex_filtration(st4, "Second move assignment from the default Simplex_tree"); - BOOST_CHECK(st3 == st4); - BOOST_CHECK(st == st4); - BOOST_CHECK(st3 == st); + Simplex_tree st7; + // To check there is no memory leak + st7.insert_simplex_and_subfaces({9, 10, 11}, 200.0); + st7 = std::move(st3); + print_simplex_filtration(st7, "First move assignment from the default Simplex_tree"); + Simplex_tree st8; + st8 = std::move(st4); + print_simplex_filtration(st8, "Second move assignment from the default Simplex_tree"); + + // Cross check + BOOST_CHECK(st7 == st8); + BOOST_CHECK(st == st8); + BOOST_CHECK(st7 == st); } -- cgit v1.2.3 From 5f4229227fe747ce08b1f296596343d9fdf79535 Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Tue, 2 Oct 2018 16:05:31 +0000 Subject: Code review : Factorize copy_from and move_from for copy/move assignment/ctor Make rec_copy private Factorize root members recursive deletion In move, test that (map_el.second.children() != &(complex_source.root_)) instead of (map_el.second.children()->oncles == &(complex_source.root_)) => more efficient Support self copy assignment detection No more use of std::swap in move assignment git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3924 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 3aa3727294e54b4ffbc48c3cdb901facf3cd59d7 --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 167 ++++++++++----------- .../test/simplex_tree_ctor_and_move_unit_test.cpp | 10 ++ 2 files changed, 89 insertions(+), 88 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 20b527a2..4df4e529 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -296,57 +296,22 @@ class Simplex_tree { dimension_(-1) { } /** \brief User-defined copy constructor reproduces the whole tree structure. */ - Simplex_tree(const Simplex_tree& complex_source) - : null_vertex_(complex_source.null_vertex_), - root_(nullptr, null_vertex_, complex_source.root_.members_), - filtration_vect_(), - dimension_(complex_source.dimension_) { + Simplex_tree(const Simplex_tree& complex_source) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree copy constructor" << std::endl; #endif // DEBUG_TRACES - auto root_source = complex_source.root_; - rec_copy(&root_, &root_source); - } - - /** \brief depth first search, inserts simplices when reaching a leaf. */ - void rec_copy(Siblings *sib, Siblings *sib_source) { - for (auto sh = sib->members().begin(), sh_source = sib_source->members().begin(); - sh != sib->members().end(); ++sh, ++sh_source) { - if (has_children(sh_source)) { - Siblings * newsib = new Siblings(sib, sh_source->first); - newsib->members_.reserve(sh_source->second.children()->members().size()); - for (auto & child : sh_source->second.children()->members()) - newsib->members_.emplace_hint(newsib->members_.end(), child.first, Node(newsib, child.second.filtration())); - rec_copy(newsib, sh_source->second.children()); - sh->second.assign_children(newsib); - } - } + copy_from(complex_source); } /** \brief User-defined move constructor relocates the whole tree structure. * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ - Simplex_tree(Simplex_tree && complex_source) - : null_vertex_(std::move(complex_source.null_vertex_)), - root_(std::move(complex_source.root_)), - filtration_vect_(std::move(complex_source.filtration_vect_)), - dimension_(std::move(complex_source.dimension_)) { + Simplex_tree(Simplex_tree && complex_source) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree move constructor" << std::endl; #endif // DEBUG_TRACES - // Need to update root members (children->oncles and children need to point on the new root pointer) - for (auto& map_el : root_.members()) { - if (map_el.second.children()->oncles() == &(complex_source.root_)) { - // reset children->oncles with the moved root_ pointer value - map_el.second.children()->oncles_ = &root_; - } else { - // if simplex is of dimension 0, oncles_ shall be nullptr - GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, - std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // and children points on root_ - to be moved - map_el.second.assign_children(&root_); - } - } + move_from(complex_source); + // just need to set dimension_ on source to make it available again // (filtration_vect_ and members are already set from the move) complex_source.dimension_ = -1; @@ -354,11 +319,7 @@ class Simplex_tree { /** \brief Destructor; deallocates the whole tree structure. */ ~Simplex_tree() { - for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { - if (has_children(sh)) { - rec_delete(sh->second.children()); - } - } + root_members_recursive_deletion(); } /** \brief User-defined copy assignment reproduces the whole tree structure. */ @@ -367,25 +328,13 @@ class Simplex_tree { #ifdef DEBUG_TRACES std::cout << "Simplex_tree copy assignment" << std::endl; #endif // DEBUG_TRACES - null_vertex_ = complex_source.null_vertex_; - filtration_vect_.clear(); - dimension_ = complex_source.dimension_; - auto root_source = complex_source.root_; - - // We start by deleting root_ if not empty - for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { - if (has_children(sh)) { - rec_delete(sh->second.children()); - } - } + // Self-assignment detection + if (&complex_source != this) { + // We start by deleting root_ if not empty + root_members_recursive_deletion(); - // root members copy - root_.members() = Dictionary(boost::container::ordered_unique_range, root_source.members().begin(), root_source.members().end()); - // Needs to reassign children - for (auto& map_el : root_.members()) { - map_el.second.assign_children(&root_); + copy_from(complex_source); } - rec_copy(&root_, &root_source); return *this; } @@ -399,37 +348,79 @@ class Simplex_tree { #endif // DEBUG_TRACES // Self-assignment detection if (&complex_source != this) { - std::swap( null_vertex_, complex_source.null_vertex_ ); - std::swap( root_, complex_source.root_ ); - std::swap( filtration_vect_, complex_source.filtration_vect_ ); - std::swap( dimension_, complex_source.dimension_ ); - - // Need to update root members (children->oncles and children need to point on the new root pointer) - for (auto& map_el : root_.members()) { - if (map_el.second.children()->oncles() == &(complex_source.root_)) { - // reset children->oncles with the moved root_ pointer value - map_el.second.children()->oncles_ = &root_; - } else { - // if simplex is of dimension 0, oncles_ shall be nullptr - GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, - std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // and children points on root_ - to be moved - map_el.second.assign_children(&root_); - } - } - // complex_source root_ deletion - for (auto sh = complex_source.root_.members().begin(); sh != complex_source.root_.members().end(); ++sh) { - if (has_children(sh)) { - rec_delete(sh->second.children()); - } - } - complex_source.root_.members().clear(); - complex_source.dimension_ = -1; + // root_ deletion in case it was not empty + root_members_recursive_deletion(); + + move_from(complex_source); } return *this; } /** @} */ // end constructor/destructor + private: + // Copy from complex_source to "this" + void copy_from(const Simplex_tree& complex_source) { + null_vertex_ = complex_source.null_vertex_; + filtration_vect_.clear(); + dimension_ = complex_source.dimension_; + auto root_source = complex_source.root_; + + // root members copy + root_.members() = Dictionary(boost::container::ordered_unique_range, root_source.members().begin(), root_source.members().end()); + // Needs to reassign children + for (auto& map_el : root_.members()) { + map_el.second.assign_children(&root_); + } + rec_copy(&root_, &root_source); + } + + /** \brief depth first search, inserts simplices when reaching a leaf. */ + void rec_copy(Siblings *sib, Siblings *sib_source) { + for (auto sh = sib->members().begin(), sh_source = sib_source->members().begin(); + sh != sib->members().end(); ++sh, ++sh_source) { + if (has_children(sh_source)) { + Siblings * newsib = new Siblings(sib, sh_source->first); + newsib->members_.reserve(sh_source->second.children()->members().size()); + for (auto & child : sh_source->second.children()->members()) + newsib->members_.emplace_hint(newsib->members_.end(), child.first, Node(newsib, child.second.filtration())); + rec_copy(newsib, sh_source->second.children()); + sh->second.assign_children(newsib); + } + } + } + + // Move from complex_source to "this" + void move_from(Simplex_tree& complex_source) { + null_vertex_ = std::move(complex_source.null_vertex_); + root_ = std::move(complex_source.root_); + filtration_vect_ = std::move(complex_source.filtration_vect_); + dimension_ = std::move(complex_source.dimension_); + + // Need to update root members (children->oncles and children need to point on the new root pointer) + for (auto& map_el : root_.members()) { + if (map_el.second.children() != &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value + map_el.second.children()->oncles_ = &root_; + } else { + // if simplex is of dimension 0, oncles_ shall be nullptr + GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, + std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); + // and children points on root_ - to be moved + map_el.second.assign_children(&root_); + } + } + } + + // delete all root_.members() recursively + void root_members_recursive_deletion() { + for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { + if (has_children(sh)) { + rec_delete(sh->second.children()); + } + } + root_.members().clear(); + } + // Recursive deletion void rec_delete(Siblings * sib) { for (auto sh = sib->members().begin(); sh != sib->members().end(); ++sh) { diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index 6c27a458..e729cf00 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -88,6 +88,11 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te BOOST_CHECK(st == st4); BOOST_CHECK(st3 == st); + st = st; + print_simplex_filtration(st4, "Third self copy assignment from the default Simplex_tree"); + + BOOST_CHECK(st3 == st); + std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE CONSTRUCTOR" << std::endl; Simplex_tree st5(std::move(st1)); @@ -124,4 +129,9 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te BOOST_CHECK(st == st8); BOOST_CHECK(st7 == st); + st = std::move(st); + print_simplex_filtration(st, "Third self move assignment from the default Simplex_tree"); + + BOOST_CHECK(st7 == st); + } -- cgit v1.2.3 From 13b6be9e73f4ac5105f8344dcf37a7007e9ef904 Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Thu, 17 Jan 2019 15:26:17 +0000 Subject: adjacency lists can be directed, undirected or bidirectional for Simplex tree insert_graph method Use of directed adjacency lists to fasten computation and use less memory git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/adjacency_list_direction_improvement@4061 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 839fabc15398ce590d09806bd783cd656029f3c1 --- src/Rips_complex/include/gudhi/Rips_complex.h | 2 +- .../include/gudhi/Sparse_rips_complex.h | 2 +- .../example/cech_complex_cgal_mini_sphere_3d.cpp | 2 +- src/Simplex_tree/include/gudhi/Simplex_tree.h | 19 +++++---- src/Simplex_tree/test/simplex_tree_unit_test.cpp | 47 +++++++++++++++------- .../include/gudhi/graph_simplicial_complex.h | 2 +- 6 files changed, 47 insertions(+), 27 deletions(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Rips_complex/include/gudhi/Rips_complex.h b/src/Rips_complex/include/gudhi/Rips_complex.h index f0fe57f4..e902e52c 100644 --- a/src/Rips_complex/include/gudhi/Rips_complex.h +++ b/src/Rips_complex/include/gudhi/Rips_complex.h @@ -59,7 +59,7 @@ class Rips_complex { /** * \brief Type of the one skeleton graph stored inside the Rips complex structure. */ - typedef typename boost::adjacency_list < boost::vecS, boost::vecS, boost::undirectedS + typedef typename boost::adjacency_list < boost::vecS, boost::vecS, boost::directedS , boost::property < vertex_filtration_t, Filtration_value > , boost::property < edge_filtration_t, Filtration_value >> OneSkeletonGraph; diff --git a/src/Rips_complex/include/gudhi/Sparse_rips_complex.h b/src/Rips_complex/include/gudhi/Sparse_rips_complex.h index 4dcc08ed..00da148f 100644 --- a/src/Rips_complex/include/gudhi/Sparse_rips_complex.h +++ b/src/Rips_complex/include/gudhi/Sparse_rips_complex.h @@ -55,7 +55,7 @@ template class Sparse_rips_complex { private: // TODO(MG): use a different graph where we know we can safely insert in parallel. - typedef typename boost::adjacency_list, boost::property> Graph; diff --git a/src/Simplex_tree/example/cech_complex_cgal_mini_sphere_3d.cpp b/src/Simplex_tree/example/cech_complex_cgal_mini_sphere_3d.cpp index 34092ef6..6bab8adb 100644 --- a/src/Simplex_tree/example/cech_complex_cgal_mini_sphere_3d.cpp +++ b/src/Simplex_tree/example/cech_complex_cgal_mini_sphere_3d.cpp @@ -50,7 +50,7 @@ using Vertex_handle = Simplex_tree::Vertex_handle; using Simplex_handle = Simplex_tree::Simplex_handle; using Filtration_value = Simplex_tree::Filtration_value; using Siblings = Simplex_tree::Siblings; -using Graph_t = boost::adjacency_list, boost::property >; using Edge_t = std::pair; diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 78697ea2..4b18651c 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -1049,7 +1049,8 @@ class Simplex_tree { * boost::graph_traits::vertex_descriptor * must be Vertex_handle. * boost::graph_traits::directed_category - * must be undirected_tag. + * can be directed_tag (the fastest, the least RAM use), undirected_tag or even + * bidirected_tag. * * If an edge appears with multiplicity, the function will arbitrarily pick * one representative to read the filtration value. */ @@ -1077,12 +1078,14 @@ class Simplex_tree { root_.members_.end(), *v_it, Node(&root_, boost::get(vertex_filtration_t(), skel_graph, *v_it))); } - typename boost::graph_traits::edge_iterator e_it, - e_it_end; - for (std::tie(e_it, e_it_end) = boost::edges(skel_graph); e_it != e_it_end; - ++e_it) { - auto u = source(*e_it, skel_graph); - auto v = target(*e_it, skel_graph); + std::pair::edge_iterator, + typename boost::graph_traits::edge_iterator> boost_edges = boost::edges(skel_graph); + // boost_edges.first is the equivalent to boost_edges.begin() + // boost_edges.second is the equivalent to boost_edges.end() + for (; boost_edges.first != boost_edges.second; boost_edges.first++) { + auto edge = *(boost_edges.first); + auto u = source(edge, skel_graph); + auto v = target(edge, skel_graph); if (u == v) throw "Self-loops are not simplicial"; // We cannot skip edges with the wrong orientation and expect them to // come a second time with the right orientation, that does not always @@ -1098,7 +1101,7 @@ class Simplex_tree { } sh->second.children()->members().emplace(v, - Node(sh->second.children(), boost::get(edge_filtration_t(), skel_graph, *e_it))); + Node(sh->second.children(), boost::get(edge_filtration_t(), skel_graph, edge))); } } diff --git a/src/Simplex_tree/test/simplex_tree_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_unit_test.cpp index f63ea080..0ae073ca 100644 --- a/src/Simplex_tree/test/simplex_tree_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_unit_test.cpp @@ -864,34 +864,51 @@ BOOST_AUTO_TEST_CASE(make_filtration_non_decreasing) { } -BOOST_AUTO_TEST_CASE(insert_graph) { + +typedef boost::mpl::list, + boost::property>, + boost::adjacency_list, + boost::property>, + boost::adjacency_list, + boost::property>> list_of_graph_variants; + +BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_tree_insert_graph, Graph, list_of_graph_variants) { std::cout << "********************************************************************" << std::endl; std::cout << "INSERT GRAPH" << std::endl; - typedef typename boost::adjacency_list, - boost::property> Graph; + Graph g(3); // filtration value 0 everywhere put(Gudhi::vertex_filtration_t(), g, 0, 0); put(Gudhi::vertex_filtration_t(), g, 1, 0); put(Gudhi::vertex_filtration_t(), g, 2, 0); // vertices don't always occur in sorted order - add_edge(0, 1, 0, g); - add_edge(2, 1, 0, g); - add_edge(2, 0, 0, g); + add_edge(0, 1, 1.1, g); + add_edge(2, 0, 2.2, g); + add_edge(2, 1, 3.3, g); - typedef Simplex_tree<> typeST; - typeST st1; + Simplex_tree<> st1; st1.insert_graph(g); BOOST_CHECK(st1.num_simplices() == 6); // edges can have multiplicity in the graph unless we replace the first vecS with (hash_)setS - add_edge(1, 0, 0, g); - add_edge(1, 2, 0, g); - add_edge(0, 2, 0, g); - add_edge(0, 2, 0, g); - typeST st2; + add_edge(1, 0, 1.1, g); + add_edge(1, 2, 3.3, g); + add_edge(0, 2, 2.2, g); + add_edge(0, 1, 1.1, g); + add_edge(2, 1, 3.3, g); + add_edge(2, 0, 2.2, g); + Simplex_tree<> st2; st2.insert_graph(g); BOOST_CHECK(st2.num_simplices() == 6); + + std::cout << "st1 is" << std::endl; + std::cout << st1 << std::endl; + + std::cout << "st2 is" << std::endl; + std::cout << st2 << std::endl; + + BOOST_CHECK(st1 == st2); } diff --git a/src/common/include/gudhi/graph_simplicial_complex.h b/src/common/include/gudhi/graph_simplicial_complex.h index 49fe56cc..0d81ca71 100644 --- a/src/common/include/gudhi/graph_simplicial_complex.h +++ b/src/common/include/gudhi/graph_simplicial_complex.h @@ -49,7 +49,7 @@ struct vertex_filtration_t { * */ template -using Proximity_graph = typename boost::adjacency_list < boost::vecS, boost::vecS, boost::undirectedS +using Proximity_graph = typename boost::adjacency_list < boost::vecS, boost::vecS, boost::directedS , boost::property < vertex_filtration_t, typename SimplicialComplexForProximityGraph::Filtration_value > , boost::property < edge_filtration_t, typename SimplicialComplexForProximityGraph::Filtration_value >>; -- cgit v1.2.3 From ddc4e972bd9a1a2517365fdacd6b23dc55148e3b Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Fri, 18 Jan 2019 10:21:40 +0000 Subject: Add setS test for adjacency_list vertices git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/adjacency_list_direction_improvement@4066 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 78e72b061f0e863617641384d7cca9291e107aac --- src/Simplex_tree/test/simplex_tree_unit_test.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'src/Simplex_tree/test') diff --git a/src/Simplex_tree/test/simplex_tree_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_unit_test.cpp index 0ae073ca..b27bbce8 100644 --- a/src/Simplex_tree/test/simplex_tree_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_unit_test.cpp @@ -865,7 +865,16 @@ BOOST_AUTO_TEST_CASE(make_filtration_non_decreasing) { } -typedef boost::mpl::list, + boost::property>, + boost::adjacency_list, + boost::property>, + boost::adjacency_list, + boost::property>, + boost::adjacency_list, boost::property>, boost::adjacency_list