From 49d24a5e4cd1f45e66f44fc27a6ff2b0268d8eb0 Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau Date: Fri, 25 Oct 2024 17:53:52 +0200 Subject: [PATCH 1/8] Copy data when Simplex_tree has the same options. No copy for transforming constructor --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 107 +++++++++++------- src/Simplex_tree/test/CMakeLists.txt | 3 + .../test/simplex_tree_unit_test.cpp | 25 +--- 3 files changed, 71 insertions(+), 64 deletions(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 45a955dd87..c0935889d4 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -61,7 +61,7 @@ namespace Gudhi { -/** \addtogroup simplex_tree +/** \addtogroup simplex_tree * @{ */ @@ -280,12 +280,12 @@ class Simplex_tree { typedef Simplex_tree_complex_simplex_iterator Complex_simplex_iterator; /** \brief Range over the simplices of the simplicial complex. */ typedef boost::iterator_range Complex_simplex_range; - /** \brief Iterator over the simplices of the skeleton of the simplicial complex, for a given + /** \brief Iterator over the simplices of the skeleton of the simplicial complex, for a given * dimension. * * 'value_type' is Simplex_handle. */ typedef Simplex_tree_skeleton_simplex_iterator Skeleton_simplex_iterator; - /** \brief Range over the simplices of the skeleton of the simplicial complex, for a given + /** \brief Range over the simplices of the skeleton of the simplicial complex, for a given * dimension. */ typedef boost::iterator_range Skeleton_simplex_range; /** \brief Range over the simplices of the simplicial complex, ordered by the filtration. */ @@ -299,7 +299,7 @@ class Simplex_tree { /** \name Range and iterator methods * @{ */ - /** \brief Returns a range over the vertices of the simplicial complex. + /** \brief Returns a range over the vertices of the simplicial complex. * The order is increasing according to < on Vertex_handles.*/ Complex_vertex_range complex_vertex_range() { return Complex_vertex_range( @@ -418,7 +418,7 @@ class Simplex_tree { * are already implicitly convertible if the concept of @ref SimplexTreeOptions is respected (note that there is * an eventual loss of precision or an undefined behaviour if a value is converted into a new type too small to * contain it). Any extra data (@ref Simplex_data) stored in the simplices are ignored in the copy for now. - * + * * @tparam OtherSimplexTreeOptions Options of the given simplex tree. * @tparam F Method taking an OtherSimplexTreeOptions::Filtration_value as input and returning an * Options::Filtration_value. @@ -509,7 +509,21 @@ class Simplex_tree { for (auto& map_el : root_.members()) { map_el.second.assign_children(&root_); } - rec_copy( + // Specific for optionnal data + if constexpr (!std::is_same_v) { + auto dst_iter = root_.members().begin(); + auto src_iter = root_source.members().begin(); + + while(dst_iter != root_.members().end() || src_iter != root_source.members().end()) { + dst_iter->second.data() = src_iter->second.data(); + dst_iter++; + src_iter++; + } + // Check in debug mode members data were copied + assert(dst_iter == root_.members().end()); + assert(src_iter == root_source.members().end()); + } + rec_copy( &root_, &root_source, [](const Filtration_value& fil) -> const Filtration_value& { return fil; }); } @@ -533,11 +547,11 @@ class Simplex_tree { } } - rec_copy(&root_, &root_source, translate_filtration_value); + rec_copy(&root_, &root_source, translate_filtration_value); } /** \brief depth first search, inserts simplices when reaching a leaf. */ - template + template void rec_copy(Siblings *sib, OtherSiblings *sib_source, F&& translate_filtration_value) { auto sh_source = sib_source->members().begin(); for (auto sh = sib->members().begin(); sh != sib->members().end(); ++sh, ++sh_source) { @@ -548,18 +562,23 @@ class Simplex_tree { newsib->members_.reserve(sh_source->second.children()->members().size()); } for (auto & child : sh_source->second.children()->members()){ + Dictionary_it new_it{}; if constexpr (store_key && Options::store_key) { - newsib->members_.emplace_hint( + new_it = newsib->members_.emplace_hint( newsib->members_.end(), child.first, Node(newsib, translate_filtration_value(child.second.filtration()), child.second.key())); } else { - newsib->members_.emplace_hint(newsib->members_.end(), + new_it = newsib->members_.emplace_hint(newsib->members_.end(), child.first, Node(newsib, translate_filtration_value(child.second.filtration()))); } + // Specific for optionnal data + if constexpr (copy_data && !std::is_same_v) { + new_it->second.data() = child.second.data(); + } } - rec_copy(newsib, sh_source->second.children(), translate_filtration_value); + rec_copy(newsib, sh_source->second.children(), translate_filtration_value); sh->second.assign_children(newsib); } } @@ -758,7 +777,7 @@ class Simplex_tree { /** * @brief Returns the dimension of the given sibling simplices. - * + * * @param curr_sib Pointer to the sibling container. * @return Height of the siblings in the tree (root counts as zero to make the height correspond to the dimension). */ @@ -918,7 +937,7 @@ class Simplex_tree { * we assign this simplex with the new value 'filtration', and set the Simplex_handle field of the * output pair to the Simplex_handle of the simplex. Otherwise, we set the Simplex_handle part to * null_simplex. - * + * */ template > std::pair insert_simplex_raw(const RandomVertexHandleRange& simplex, @@ -1229,7 +1248,7 @@ class Simplex_tree { * \param simplex represent the simplex of which we search the star * \return Vector of Simplex_tree::Simplex_handle (empty vector if no star found) when * SimplexTreeOptions::link_nodes_by_label is false. - * + * * Simplex_tree::Simplex_handle range for an optimized search for the star of a simplex when * SimplexTreeOptions::link_nodes_by_label is true. */ @@ -1243,7 +1262,7 @@ class Simplex_tree { * cofaces (equivalent of star function) * \return Vector of Simplex_tree::Simplex_handle (empty vector if no cofaces found) when * SimplexTreeOptions::link_nodes_by_label is false. - * + * * Simplex_tree::Simplex_handle range for an optimized search for the coface of a simplex when * SimplexTreeOptions::link_nodes_by_label is true. */ @@ -1452,7 +1471,7 @@ class Simplex_tree { * @brief Adds a new vertex or a new edge in a flag complex, as well as all * simplices of its star, defined to maintain the property * of the complex to be a flag complex, truncated at dimension dim_max. - * To insert a new edge, the two given vertex handles have to correspond + * To insert a new edge, the two given vertex handles have to correspond * to the two end points of the edge. To insert a new vertex, the handles * have to be twice the same and correspond to the number you want assigned * to it. I.e., to insert vertex \f$i\f$, give \f$u = v = i\f$. @@ -1460,7 +1479,7 @@ class Simplex_tree { * the simplex tree, so the behaviour is undefined if called on an existing * edge. Also, the vertices of an edge have to be inserted before the edge. * - * @param[in] u,v Vertex_handle representing the new edge + * @param[in] u,v Vertex_handle representing the new edge * (@p v != @p u) or the new vertex (@p v == @p u). * @param[in] fil Filtration value of the edge. * @param[in] dim_max Maximal dimension of the expansion. @@ -1478,8 +1497,8 @@ class Simplex_tree { * filtration values, the method `make_filtration_non_decreasing()` has to be * called at the end of the insertions to restore the intended filtration. * Note that even then, an edge has to be inserted after its vertices. - * @warning The method assumes that the given edge or vertex was not already - * contained in the simplex tree, so the behaviour is undefined if called on + * @warning The method assumes that the given edge or vertex was not already + * contained in the simplex tree, so the behaviour is undefined if called on * an existing simplex. */ void insert_edge_as_flag( Vertex_handle u @@ -2084,7 +2103,7 @@ class Simplex_tree { bool prune_above_dimension(int dimension) { if (dimension >= dimension_) return false; - + bool modified = false; if (dimension < 0) { if (num_vertices() > 0) { @@ -2185,17 +2204,17 @@ class Simplex_tree { } } - /** \brief Retrieve the original filtration value for a given simplex in the Simplex_tree. Since the + /** \brief Retrieve the original filtration value for a given simplex in the Simplex_tree. Since the * computation of extended persistence requires modifying the filtration values, this function can be used * to recover the original values. Moreover, computing extended persistence requires adding new simplices * in the Simplex_tree. Hence, this function also outputs the type of each simplex. It can be either UP (which means * that the simplex was present originally, and is thus part of the ascending extended filtration), DOWN (which means * that the simplex is the cone of an original simplex, and is thus part of the descending extended filtration) or * EXTRA (which means the simplex is the cone point). See the definition of Extended_simplex_type. Note that if the simplex type is DOWN, the original filtration value - * is set to be the original filtration value of the corresponding (not coned) original simplex. + * is set to be the original filtration value of the corresponding (not coned) original simplex. * \pre This function should be called only if `extend_filtration()` has been called first! * \post The output filtration value is supposed to be the same, but might be a little different, than the - * original filtration value, due to the internal transformation (scaling to [-2,-1]) that is + * original filtration value, due to the internal transformation (scaling to [-2,-1]) that is * performed on the original filtration values during the computation of extended persistence. * @param[in] f Filtration value of the simplex in the extended (i.e., modified) filtration. * @param[in] efd Structure containing the minimum and maximum values of the original filtration. This the output of `extend_filtration()`. @@ -2215,12 +2234,12 @@ class Simplex_tree { return p; }; - /** \brief Extend filtration for computing extended persistence. - * This function only uses the filtration values at the 0-dimensional simplices, - * and computes the extended persistence diagram induced by the lower-star filtration - * computed with these values. - * \post Note that after calling this function, the filtration - * values are actually modified. The function `decode_extended_filtration()` + /** \brief Extend filtration for computing extended persistence. + * This function only uses the filtration values at the 0-dimensional simplices, + * and computes the extended persistence diagram induced by the lower-star filtration + * computed with these values. + * \post Note that after calling this function, the filtration + * values are actually modified. The function `decode_extended_filtration()` * retrieves the original values and outputs the extended simplex type. * @exception std::invalid_argument In debug mode if the Simplex tree contains a vertex with the largest * Vertex_handle, as this method requires to create an extra vertex internally. @@ -2241,7 +2260,7 @@ class Simplex_tree { maxval = std::max(maxval, f); maxvert = std::max(sh->first, maxvert); } - + GUDHI_CHECK(maxvert < std::numeric_limits::max(), std::invalid_argument("Simplex_tree contains a vertex with the largest Vertex_handle")); maxvert++; @@ -2280,7 +2299,7 @@ class Simplex_tree { // Automatically assign good values for simplices this->make_filtration_non_decreasing(); - // Return the filtration data + // Return the filtration data return Extended_filtration_data(minval, maxval); } @@ -2398,14 +2417,14 @@ class Simplex_tree { //decltype(testPtr) = boost::intrusive::compact_rbtree_node* decltype(testPtr) sh_ptr = decltype(testPtr)((const char*)(&node) - shift); //shifts from node to pointer - //decltype(testIIt) = + //decltype(testIIt) = //boost::intrusive::tree_iterator< // boost::intrusive::bhtraits< // boost::container::base_node< // std::pair>, // boost::container::dtl::intrusive_tree_hook, true>, - // boost::intrusive::rbtree_node_traits, - // boost::intrusive::normal_link, + // boost::intrusive::rbtree_node_traits, + // boost::intrusive::normal_link, // boost::intrusive::dft_tag, // 3>, // false> @@ -2482,9 +2501,9 @@ class Simplex_tree { public: /** @private @brief Returns the serialization required buffer size. - * + * * @return The exact serialization required size in number of bytes. - * + * * @warning It is meant to return the same size with the same SimplexTreeOptions and on a computer with the same * architecture. */ @@ -2497,14 +2516,14 @@ class Simplex_tree { #endif // DEBUG_TRACES return buffer_byte_size; } - + /** @private @brief Serialize the Simplex tree - Flatten it in a user given array of char - * + * * @param[in] buffer An array of char allocated with enough space (cf. Gudhi::simplex_tree::get_serialization_size) * @param[in] buffer_size The buffer size. - * + * * @exception std::invalid_argument If serialization does not match exactly the buffer_size value. - * + * * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. */ @@ -2565,16 +2584,16 @@ class Simplex_tree { public: /** @private @brief Deserialize the array of char (flatten version of the tree) to initialize a Simplex tree. * It is the user's responsibility to provide an 'empty' Simplex_tree, there is no guarantee otherwise. - * + * * @param[in] buffer A pointer on a buffer that contains a serialized Simplex_tree. * @param[in] buffer_size The size of the buffer. - * + * * @exception std::invalid_argument In case the deserialization does not finish at the correct buffer_size. * @exception std::logic_error In debug mode, if the Simplex_tree is not 'empty'. - * + * * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. - * + * */ void deserialize(const char* buffer, const std::size_t buffer_size) { GUDHI_CHECK(num_vertices() == 0, std::logic_error("Simplex_tree::deserialize - Simplex_tree must be empty")); diff --git a/src/Simplex_tree/test/CMakeLists.txt b/src/Simplex_tree/test/CMakeLists.txt index 5f441b338b..55160603af 100644 --- a/src/Simplex_tree/test/CMakeLists.txt +++ b/src/Simplex_tree/test/CMakeLists.txt @@ -29,3 +29,6 @@ gudhi_add_boost_test(Simplex_tree_edge_expansion_unit_test) add_executable_with_targets(Simplex_tree_extended_filtration_test_unit simplex_tree_extended_filtration_unit_test.cpp TBB::tbb) gudhi_add_boost_test(Simplex_tree_extended_filtration_test_unit) + +add_executable_with_targets(Simplex_tree_data_test_unit simplex_tree_data_unit_test.cpp TBB::tbb) +gudhi_add_boost_test(Simplex_tree_data_test_unit) diff --git a/src/Simplex_tree/test/simplex_tree_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_unit_test.cpp index d0dc0140ec..31f6296ab0 100644 --- a/src/Simplex_tree/test/simplex_tree_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_unit_test.cpp @@ -498,7 +498,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(NSimplexAndSubfaces_tree_insertion, typeST, list_o BOOST_CHECK(vertex == SimplexVector6[position]); position++; } - + /* Inserted simplex: */ /* 1 6 */ /* o---o */ @@ -818,7 +818,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(copy_move_on_simplex_tree, typeST, list_of_tested_ std::clog << "Printing st - address = " << &st << std::endl; - // Copy constructor + // Copy constructor typeST st_copy = st; std::clog << "Printing a copy of st - address = " << &st_copy << std::endl; @@ -827,7 +827,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(copy_move_on_simplex_tree, typeST, list_of_tested_ // Check there is a new simplex tree reference BOOST_CHECK(&st != &st_copy); - // Move constructor + // Move constructor typeST st_move = std::move(st); std::clog << "Printing a move of st - address = " << &st_move << std::endl; @@ -836,14 +836,14 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(copy_move_on_simplex_tree, typeST, list_of_tested_ // Check there is a new simplex tree reference BOOST_CHECK(&st_move != &st_copy); BOOST_CHECK(&st_move != &st); - + typeST st_empty; // Check st has been emptied by the move BOOST_CHECK(st == st_empty); BOOST_CHECK(st.dimension() == -1); BOOST_CHECK(st.num_simplices() == 0); BOOST_CHECK(st.num_vertices() == (size_t)0); - + std::clog << "Printing st once again- address = " << &st << std::endl; } @@ -1227,18 +1227,3 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(for_each_simplex_skip_iteration, typeST, list_of_t BOOST_CHECK(num_simplices_by_dim_until_two[0] == num_simplices_by_dim[0]); BOOST_CHECK(num_simplices_by_dim_until_two[1] == num_simplices_by_dim[1]); } - -struct Options_with_int_data : Simplex_tree_options_minimal { - typedef int Simplex_data; -}; - -BOOST_AUTO_TEST_CASE(simplex_data) { - Simplex_tree st; - st.insert_simplex_and_subfaces({0, 1}); - st.insert_simplex_and_subfaces({2, 1}); - st.insert_simplex_and_subfaces({0, 2}); - st.simplex_data(st.find({0, 1})) = 5; - st.expansion(3); - st.simplex_data(st.find({0, 1, 2})) = 4; - BOOST_CHECK(st.simplex_data(st.find({0, 1})) == 5); -} From 9ba4aa9de314cea6b4726be56e0b788509367ddf Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau Date: Fri, 25 Oct 2024 17:55:47 +0200 Subject: [PATCH 2/8] test was splitted and forgotten --- .../test/simplex_tree_data_unit_test.cpp | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 src/Simplex_tree/test/simplex_tree_data_unit_test.cpp diff --git a/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp new file mode 100644 index 0000000000..cc74220dac --- /dev/null +++ b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp @@ -0,0 +1,78 @@ +/* This file is part of the Gudhi Library - https://gudhi.inria.fr/ - which is released under MIT. + * See file LICENSE or go to https://gudhi.inria.fr/licensing/ for full license details. + * Author(s): Marc Glisse, Vincent Rouvreau + * + * Copyright (C) 2024 Inria + * + * Modification(s): + * - 2024/11 Vincent Rouvreau: Move data test from simplex_tree_unit_test + * - YYYY/MM Author: Description of the modification + */ + +#include +#include + +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_MODULE "simplex_tree_data" +#include + +#include "gudhi/Simplex_tree.h" + +using namespace Gudhi; + +struct Options_with_int_data : Simplex_tree_options_minimal { + typedef int Simplex_data; +}; + +BOOST_AUTO_TEST_CASE(simplex_data) { + Simplex_tree st; + st.insert_simplex_and_subfaces({0, 1}); + st.insert_simplex_and_subfaces({2, 1}); + st.insert_simplex_and_subfaces({0, 2}); + st.simplex_data(st.find({0, 1})) = 5; + st.expansion(3); + st.simplex_data(st.find({0, 1, 2})) = 4; + BOOST_CHECK(st.simplex_data(st.find({0, 1})) == 5); +} + +struct Options_with_string_data : Simplex_tree_options_full_featured { + using Simplex_data = std::string; +}; + +BOOST_AUTO_TEST_CASE(simplex_data_copy) { + Simplex_tree stree; + stree.insert_simplex_and_subfaces({0, 1}, 1.); + stree.simplex_data(stree.find({0, 1})) = "{0, 1}"; + // vertices have a specific algorithm, so let's test one data on a vertex + stree.simplex_data(stree.find({0})) = "{0}"; + stree.insert_simplex_and_subfaces({2, 1}, 2.); + stree.simplex_data(stree.find({1, 2})) = "{1, 2}"; + stree.insert_simplex_and_subfaces({0, 2}, 3.); + stree.simplex_data(stree.find({0, 2})) = "{0, 2}"; + std::clog << "Iterator on stree:\n"; + for (auto simplex : stree.complex_simplex_range()) { + std::clog << "("; + for (auto vertex : stree.simplex_vertex_range(simplex)) { + std::clog << vertex << " "; + } + std::clog << ") filtration=" << stree.filtration(simplex) << " - data='" << stree.simplex_data(simplex) << "'\n"; + } + Simplex_tree stree_copy(stree); + std::clog << "\nIterator on copy:\n"; + for (auto simplex : stree_copy.complex_simplex_range()) { + std::vector::Vertex_handle> vec_of_vertices {}; + std::clog << "("; + for (auto vertex : stree_copy.simplex_vertex_range(simplex)) { + std::clog << vertex << " "; + vec_of_vertices.push_back(vertex); + } + std::clog << ")"; + auto sh = stree.find(vec_of_vertices); + BOOST_CHECK(sh != stree.null_simplex()); + std::clog << " filtration=" << stree_copy.filtration(simplex) << " versus " << stree.filtration(sh); + std::clog << " - data='" << stree_copy.simplex_data(simplex) << "' versus '" << stree.simplex_data(sh) << "'\n"; + BOOST_CHECK(stree_copy.filtration(simplex) == stree.filtration(sh)); + BOOST_CHECK(stree_copy.simplex_data(simplex) == stree.simplex_data(sh)); + } + +} From 3c0dc0362ac9e90727cb421b330e86050c331e66 Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau Date: Tue, 5 Nov 2024 11:11:39 +0100 Subject: [PATCH 3/8] Add some tests and rename copy_data as copy_simplex_data --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 21 ++++--- .../test/simplex_tree_data_unit_test.cpp | 60 +++++++++++++------ 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index c0935889d4..76f9351799 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -11,6 +11,7 @@ * - 2023/05 Hannah Schreiber: Factorization of expansion methods * - 2023/08 Hannah Schreiber (& Clément Maria): Add possibility of stable simplex handles. * - 2024/08 Hannah Schreiber: Addition of customizable copy constructor. + * - 2024/08 Marc Glisse: Allow storing custom data in simplices. * - YYYY/MM Author: Description of the modification */ @@ -434,7 +435,9 @@ class Simplex_tree { copy_from(complex_source, translate_filtration_value); } - /** \brief User-defined copy constructor reproduces the whole tree structure. */ + /** \brief User-defined copy constructor reproduces the whole tree structure including extra data (@ref Simplex_data) + * stored in the simplices. + */ Simplex_tree(const Simplex_tree& complex_source) { #ifdef DEBUG_TRACES std::clog << "Simplex_tree copy constructor" << std::endl; @@ -442,7 +445,8 @@ class Simplex_tree { copy_from(complex_source); } - /** \brief User-defined move constructor relocates the whole tree structure. + /** \brief User-defined move constructor relocates the whole tree structure including extra data (@ref Simplex_data) + * stored in the simplices. * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ Simplex_tree(Simplex_tree && complex_source) { @@ -461,7 +465,9 @@ class Simplex_tree { root_members_recursive_deletion(); } - /** \brief User-defined copy assignment reproduces the whole tree structure. */ + /** \brief User-defined copy assignment reproduces the whole tree structure including extra data (@ref Simplex_data) + * stored in the simplices. + */ Simplex_tree& operator= (const Simplex_tree& complex_source) { #ifdef DEBUG_TRACES std::clog << "Simplex_tree copy assignment" << std::endl; @@ -476,7 +482,8 @@ class Simplex_tree { return *this; } - /** \brief User-defined move assignment relocates the whole tree structure. + /** \brief User-defined move assignment relocates the whole tree structure including extra data (@ref Simplex_data) + * stored in the simplices. * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ Simplex_tree& operator=(Simplex_tree&& complex_source) { @@ -551,7 +558,7 @@ class Simplex_tree { } /** \brief depth first search, inserts simplices when reaching a leaf. */ - template + template void rec_copy(Siblings *sib, OtherSiblings *sib_source, F&& translate_filtration_value) { auto sh_source = sib_source->members().begin(); for (auto sh = sib->members().begin(); sh != sib->members().end(); ++sh, ++sh_source) { @@ -574,11 +581,11 @@ class Simplex_tree { Node(newsib, translate_filtration_value(child.second.filtration()))); } // Specific for optionnal data - if constexpr (copy_data && !std::is_same_v) { + if constexpr (copy_simplex_data && !std::is_same_v) { new_it->second.data() = child.second.data(); } } - rec_copy(newsib, sh_source->second.children(), translate_filtration_value); + rec_copy(newsib, sh_source->second.children(), translate_filtration_value); sh->second.assign_children(newsib); } } diff --git a/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp index cc74220dac..5bf5c6175d 100644 --- a/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp @@ -11,6 +11,7 @@ #include #include +#include // for std::move #define BOOST_TEST_DYN_LINK #define BOOST_TEST_MODULE "simplex_tree_data" @@ -39,6 +40,25 @@ struct Options_with_string_data : Simplex_tree_options_full_featured { using Simplex_data = std::string; }; +template +void test_filtration_and_additionnal_data(Simplex_tree_source stree, Simplex_tree_copy stree_copy) { + for (auto simplex : stree_copy.complex_simplex_range()) { + std::vector vec_of_vertices {}; + std::clog << "("; + for (auto vertex : stree_copy.simplex_vertex_range(simplex)) { + std::clog << vertex << " "; + vec_of_vertices.push_back(vertex); + } + std::clog << ")"; + auto sh = stree.find(vec_of_vertices); + BOOST_CHECK(sh != stree.null_simplex()); + std::clog << " filtration=" << stree_copy.filtration(simplex) << " versus " << stree.filtration(sh); + std::clog << " - data='" << stree_copy.simplex_data(simplex) << "' versus '" << stree.simplex_data(sh) << "'\n"; + BOOST_CHECK(stree_copy.filtration(simplex) == stree.filtration(sh)); + BOOST_CHECK(stree_copy.simplex_data(simplex) == stree.simplex_data(sh)); + } +} + BOOST_AUTO_TEST_CASE(simplex_data_copy) { Simplex_tree stree; stree.insert_simplex_and_subfaces({0, 1}, 1.); @@ -57,22 +77,28 @@ BOOST_AUTO_TEST_CASE(simplex_data_copy) { } std::clog << ") filtration=" << stree.filtration(simplex) << " - data='" << stree.simplex_data(simplex) << "'\n"; } - Simplex_tree stree_copy(stree); - std::clog << "\nIterator on copy:\n"; - for (auto simplex : stree_copy.complex_simplex_range()) { - std::vector::Vertex_handle> vec_of_vertices {}; - std::clog << "("; - for (auto vertex : stree_copy.simplex_vertex_range(simplex)) { - std::clog << vertex << " "; - vec_of_vertices.push_back(vertex); - } - std::clog << ")"; - auto sh = stree.find(vec_of_vertices); - BOOST_CHECK(sh != stree.null_simplex()); - std::clog << " filtration=" << stree_copy.filtration(simplex) << " versus " << stree.filtration(sh); - std::clog << " - data='" << stree_copy.simplex_data(simplex) << "' versus '" << stree.simplex_data(sh) << "'\n"; - BOOST_CHECK(stree_copy.filtration(simplex) == stree.filtration(sh)); - BOOST_CHECK(stree_copy.simplex_data(simplex) == stree.simplex_data(sh)); + { + Simplex_tree stree_copy(stree); + std::clog << "\nIterator on copy constructor:\n"; + test_filtration_and_additionnal_data(stree, stree_copy); + } + { + Simplex_tree stree_copy = stree; + std::clog << "\nIterator on copy assignment:\n"; + test_filtration_and_additionnal_data(stree, stree_copy); + } + { + // Needs to copy first for not to lose the original one + Simplex_tree stree_copy = stree; + Simplex_tree stree_moved(std::move(stree_copy)); + std::clog << "\nIterator on move constructor:\n"; + test_filtration_and_additionnal_data(stree, stree_moved); + } + { + // Needs to copy first for not to lose the original one + Simplex_tree stree_copy = stree; + Simplex_tree stree_moved = std::move(stree_copy); + std::clog << "\nIterator on move assignment:\n"; + test_filtration_and_additionnal_data(stree, stree_moved); } - } From c5cddc30f35b3e4c410241ca7a03aabdc55f1363 Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau Date: Thu, 7 Nov 2024 10:24:30 +0100 Subject: [PATCH 4/8] Add a non really satisfying test for transformer constructor and data --- .../test/simplex_tree_data_unit_test.cpp | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp index 5bf5c6175d..061e4f136a 100644 --- a/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_data_unit_test.cpp @@ -33,6 +33,14 @@ BOOST_AUTO_TEST_CASE(simplex_data) { st.simplex_data(st.find({0, 1})) = 5; st.expansion(3); st.simplex_data(st.find({0, 1, 2})) = 4; + for (auto simplex : st.complex_simplex_range()) { + std::clog << "("; + for (auto vertex : st.simplex_vertex_range(simplex)) { + std::clog << vertex << " "; + } + std::clog << ")"; + std::clog << " filtration=" << st.filtration(simplex) << " - data='" << st.simplex_data(simplex) << "'\n"; + } BOOST_CHECK(st.simplex_data(st.find({0, 1})) == 5); } @@ -101,4 +109,35 @@ BOOST_AUTO_TEST_CASE(simplex_data_copy) { std::clog << "\nIterator on move assignment:\n"; test_filtration_and_additionnal_data(stree, stree_moved); } + { + auto trans = [](const typename Simplex_tree::Filtration_value& fil) + -> Simplex_tree::Filtration_value { + // spoiler: Options_with_int_data does not store filtrations + if constexpr(Options_with_int_data::store_filtration) + return fil; + else + return {}; + }; + Simplex_tree stree_copy(stree, trans); + std::clog << "\nIterator on transformer copy constructor:\n"; + Simplex_tree::Filtration_value default_filtration_value{}; + Simplex_tree::Simplex_data default_data_value{}; + for (auto simplex : stree_copy.complex_simplex_range()) { + std::vector::Vertex_handle> vec_of_vertices {}; + std::clog << "("; + for (auto vertex : stree_copy.simplex_vertex_range(simplex)) { + std::clog << vertex << " "; + vec_of_vertices.push_back(vertex); + } + std::clog << ")"; + auto sh = stree.find(vec_of_vertices); + BOOST_CHECK(sh != stree.null_simplex()); + std::clog << " filtration=" << stree_copy.filtration(simplex) << " versus " << stree.filtration(sh); + std::clog << " - data='" << stree_copy.simplex_data(simplex) << "' versus '" << stree.simplex_data(sh) << "'\n"; + BOOST_CHECK(stree_copy.filtration(simplex) == default_filtration_value); + // With transformer constructors, simplex_data is not copied and not initialized, so we cannot test it + // I just keep it in comment if we find something smarter + // BOOST_CHECK(stree_copy.simplex_data(simplex) == default_data_value); + } + } } From 7ce085d8333fa309c0111aae35aa2fa44d115d9a Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau Date: Thu, 7 Nov 2024 10:25:21 +0100 Subject: [PATCH 5/8] Add more details for comparison operator and serialization --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 76f9351799..566803d5d4 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -638,7 +638,9 @@ class Simplex_tree { public: template friend class Simplex_tree; - /** \brief Checks if two simplex trees are equal. */ + /** \brief Checks if two simplex trees are equal. Any extra data (@ref Simplex_data) stored in the simplices are + * ignored in the coomparison + */ template bool operator==(Simplex_tree& st2) { if ((null_vertex_ != st2.null_vertex_) || @@ -2533,6 +2535,8 @@ class Simplex_tree { * * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. + * + * Serialize/Deserialize ignore any extra data (@ref Simplex_data) stored in the simplices for now. */ /* Let's take the following simplicial complex as example: */ /* (vertices are represented as letters to ease the understanding) */ @@ -2601,6 +2605,7 @@ class Simplex_tree { * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. * + * Serialize/Deserialize ignore any extra data (@ref Simplex_data) stored in the simplices for now. */ void deserialize(const char* buffer, const std::size_t buffer_size) { GUDHI_CHECK(num_vertices() == 0, std::logic_error("Simplex_tree::deserialize - Simplex_tree must be empty")); From 2ab4bff4ce31e883ac5b877a22c2686955b63209 Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau <10407034+VincentRouvreau@users.noreply.github.com> Date: Thu, 14 Nov 2024 08:55:40 +0100 Subject: [PATCH 6/8] doc review: typo Co-authored-by: hschreiber <48448038+hschreiber@users.noreply.github.com> --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 566803d5d4..39e0d41395 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -639,7 +639,7 @@ class Simplex_tree { template friend class Simplex_tree; /** \brief Checks if two simplex trees are equal. Any extra data (@ref Simplex_data) stored in the simplices are - * ignored in the coomparison + * ignored in the comparison. */ template bool operator==(Simplex_tree& st2) { From 59ce648275f09c980056da51371fed8fb5bd480f Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau <10407034+VincentRouvreau@users.noreply.github.com> Date: Thu, 14 Nov 2024 08:55:54 +0100 Subject: [PATCH 7/8] doc review: typo Co-authored-by: hschreiber <48448038+hschreiber@users.noreply.github.com> --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 39e0d41395..731898dd0c 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -2605,7 +2605,7 @@ class Simplex_tree { * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. * - * Serialize/Deserialize ignore any extra data (@ref Simplex_data) stored in the simplices for now. + * Serialize/Deserialize ignores any extra data (@ref Simplex_data) stored in the simplices for now. */ void deserialize(const char* buffer, const std::size_t buffer_size) { GUDHI_CHECK(num_vertices() == 0, std::logic_error("Simplex_tree::deserialize - Simplex_tree must be empty")); From 5b3e30e768b006db0f4256b6b7b233727e97497a Mon Sep 17 00:00:00 2001 From: Vincent Rouvreau <10407034+VincentRouvreau@users.noreply.github.com> Date: Thu, 14 Nov 2024 08:56:09 +0100 Subject: [PATCH 8/8] doc review: typo Co-authored-by: hschreiber <48448038+hschreiber@users.noreply.github.com> --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 731898dd0c..1ee893402c 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -2536,7 +2536,7 @@ class Simplex_tree { * @warning Serialize/Deserialize is not portable. It is meant to be read in a Simplex_tree with the same * SimplexTreeOptions and on a computer with the same architecture. * - * Serialize/Deserialize ignore any extra data (@ref Simplex_data) stored in the simplices for now. + * Serialize/Deserialize ignores any extra data (@ref Simplex_data) stored in the simplices for now. */ /* Let's take the following simplicial complex as example: */ /* (vertices are represented as letters to ease the understanding) */