Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework simplex tree options #978

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5326b9a
Move simplex tree options in a separated header
VincentRouvreau Sep 29, 2023
c0d3d1b
Move simplex tree options in a separated header. No doc for Simplex_t…
VincentRouvreau Sep 29, 2023
f9c231e
Default (replacing full_feature as default), minimal and full_feature…
VincentRouvreau Oct 2, 2023
99fc4dd
Merge branch 'master' into rework_simplex_tree_options
VincentRouvreau Oct 16, 2023
8d2cf62
Merge remote-tracking branch 'upstream/master' into rework_simplex_tr…
VincentRouvreau Oct 19, 2023
73e62bd
Fix simplex_tree_edge_expansion_unit_test after its merge
VincentRouvreau Oct 19, 2023
9c6bdc2
code review: this was not part of a code review, but inspired from so…
VincentRouvreau Oct 19, 2023
ae70563
code review: custom simplex tree options for the cofaces access bench…
VincentRouvreau Oct 19, 2023
69e7956
code review: More comments
VincentRouvreau Oct 19, 2023
64cebe9
code review: Add some details on Simplex_tree_options_default
VincentRouvreau Oct 19, 2023
b534bcf
code review: activate stable simplex handles on full featured options
VincentRouvreau Oct 19, 2023
0dea35e
Update src/Simplex_tree/include/gudhi/Simplex_tree/simplex_tree_optio…
VincentRouvreau Oct 19, 2023
364fcd5
Merge branch 'rework_simplex_tree_options' of github.com:VincentRouvr…
VincentRouvreau Oct 19, 2023
55f9f8e
code review: use Simplex_tree_interface instead of Simplex_tree<Simpl…
VincentRouvreau Oct 19, 2023
0a82c1e
code review: rework comments about Simplex tree options
VincentRouvreau Oct 20, 2023
a6c5e0a
code review: remove insert_simplex_and_subfaces as not used and incom…
VincentRouvreau Oct 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Persistent_cohomology/example/plain_homology.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
#include <cstdint> // for std::uint8_t

/* We could perfectly well use the default Simplex_tree<> (which uses
* Simplex_tree_options_full_featured), the following simply demonstrates
* Simplex_tree_options_default), the following simply demonstrates
* how to save on storage by not storing a filtration value. */

struct MyOptions : Gudhi::Simplex_tree_options_full_featured {
struct MyOptions : Gudhi::Simplex_tree_options_minimal {
// Implicitly use 0 as filtration value for all simplices
static const bool store_filtration = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should comment out that line (keep it but make it a comment), since this is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we should not derive from any existing options and set all the options we need for this example.
Tell me your preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in an example, deriving from something is good, that's what we want to recommend.

Copy link
Contributor Author

@VincentRouvreau VincentRouvreau Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase simplex tree options for this example on 0a82c1e

// The persistence algorithm needs this
Expand Down
2 changes: 1 addition & 1 deletion src/Persistent_cohomology/test/betti_numbers_unit_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include <gudhi/Simplex_tree.h>
#include <gudhi/Persistent_cohomology.h>

struct MiniSTOptions : Gudhi::Simplex_tree_options_full_featured {
struct MiniSTOptions : Gudhi::Simplex_tree_options_minimal {
// Implicitly use 0 as filtration value for all simplices
static const bool store_filtration = false;
// The persistence algorithm needs this
Expand Down
4 changes: 2 additions & 2 deletions src/Simplex_tree/benchmark/simplex_tree_cofaces_benchmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ int main(int argc, char *argv[]) {
nb_vertices = atoi(argv[1]);

std::clog << "** Without cofaces computation optimization" << std::endl;
benchmark_stars_computation<Gudhi::Simplex_tree<Gudhi::Simplex_tree_options_full_featured>>(nb_vertices);
benchmark_stars_computation<Gudhi::Simplex_tree<Gudhi::Simplex_tree_options_default>>(nb_vertices);

std::clog << "** With cofaces computation optimization" << std::endl;
benchmark_stars_computation<Gudhi::Simplex_tree<Gudhi::Simplex_tree_options_fast_cofaces>>(nb_vertices);
benchmark_stars_computation<Gudhi::Simplex_tree<Gudhi::Simplex_tree_options_full_featured>>(nb_vertices);
mglisse marked this conversation as resolved.
Show resolved Hide resolved

std::clog << "** With cofaces computation optimization and stable simplex handles" << std::endl;
benchmark_stars_computation<Gudhi::Simplex_tree<Simplex_tree_options_stable_simplex_handles> >(nb_vertices);
Expand Down
28 changes: 17 additions & 11 deletions src/Simplex_tree/concept/SimplexTreeOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,34 @@
* - YYYY/MM Author: Description of the modification
*/

/** \brief Concept of the template parameter for the class `Gudhi::Simplex_tree<SimplexTreeOptions>`.
/** @brief Concept of the template parameter for the class `Gudhi::Simplex_tree<SimplexTreeOptions>`.
*
* One model for this is `Gudhi::Simplex_tree_options_full_featured`. If you want to provide your own, it is recommended that you derive from it and override some parts instead of writing a class from scratch.
* A model for this is `Gudhi::Simplex_tree_options_full_featured` or `Gudhi::Simplex_tree_options_minimal`.
* If you want to provide your own, it is recommended that you derive from it and override some parts instead of
* writing a class from scratch.
*/
struct SimplexTreeOptions {
/// Forced for now.
/** @brief Forced for now. */
typedef IndexingTag Indexing_tag;
/// Must be a signed integer type. It admits a total order <.
/** @brief Must be a signed integer type. It admits a total order <. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not this PR, but at some point, for ourselves (not necessarily for users), it would be nice to have a comment explaining where such requirements ("signed") come from.

typedef VertexHandle Vertex_handle;
/// Must be comparable with operator<.
/** @brief Must be comparable with operator<. */
typedef FiltrationValue Filtration_value;
/// Must be an integer type.
/** @brief Must be an integer type. */
typedef SimplexKey Simplex_key;
/// If true, each simplex has extra storage for one `Simplex_key`. Necessary for `Persistent_cohomology`.
/** @brief If true, each simplex has extra storage for one `Simplex_key`. Necessary for `Persistent_cohomology`. */
static const bool store_key;
/// If true, each simplex has extra storage for one `Filtration_value`, and this value is propagated by operations like `Gudhi::Simplex_tree::expansion`. Without it, `Persistent_cohomology` degenerates to computing usual (non-persistent) cohomology.
/** @brief If true, each simplex has extra storage for one `Filtration_value`, and this value is propagated by
* operations like `Gudhi::Simplex_tree::expansion`. Without it, `Persistent_cohomology` degenerates to computing
* usual (non-persistent) cohomology.
*/
static const bool store_filtration;
/// If true, the list of vertices present in the complex must always be 0, ..., num_vertices-1, without any hole.

/** @brief If true, the list of vertices present in the complex must always be 0, ..., num_vertices-1, without any hole. */
static constexpr bool contiguous_vertices;
/// If true, the lists of `Node` with same label are stored to enhance cofaces and stars access.
/** @brief If true, the lists of `Node` with same label are stored to enhance cofaces and stars access. */
static const bool link_nodes_by_label;
/// If true, Simplex_handle will not be invalidated after insertions or removals.
/** @brief If true, Simplex_handle will not be invalidated after insertions or removals. */
static const bool stable_simplex_handles;
};

2 changes: 1 addition & 1 deletion src/Simplex_tree/example/mini_simplex_tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include <iostream>
#include <initializer_list>

struct MyOptions : Gudhi::Simplex_tree_options_full_featured {
struct MyOptions : Gudhi::Simplex_tree_options_minimal {
// Not doing persistence, so we don't need those
static const bool store_key = false;
static const bool store_filtration = false;
mglisse marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
57 changes: 2 additions & 55 deletions src/Simplex_tree/include/gudhi/Simplex_tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
#ifndef SIMPLEX_TREE_H_
#define SIMPLEX_TREE_H_

#include <gudhi/Simplex_tree/simplex_tree_options.h>
#include <gudhi/Simplex_tree/Simplex_tree_node_explicit_storage.h>
#include <gudhi/Simplex_tree/Simplex_tree_siblings.h>
#include <gudhi/Simplex_tree/Simplex_tree_iterators.h>
#include <gudhi/Simplex_tree/Simplex_tree_star_simplex_iterators.h>
#include <gudhi/Simplex_tree/indexing_tag.h>
#include <gudhi/Simplex_tree/serialization_utils.h> // for Gudhi::simplex_tree::de/serialize_trivial
#include <gudhi/Simplex_tree/hooks_simplex_base.h>

Expand Down Expand Up @@ -50,7 +50,6 @@
#include <limits> // Inf
#include <initializer_list>
#include <algorithm> // for std::max
#include <cstdint> // for std::uint32_t
#include <iterator> // for std::distance
#include <type_traits> // for std::conditional
#include <unordered_map>
Expand All @@ -76,8 +75,6 @@ namespace Gudhi {
*/
enum class Extended_simplex_type {UP, DOWN, EXTRA};

struct Simplex_tree_options_full_featured;

/**
* \class Simplex_tree Simplex_tree.h gudhi/Simplex_tree.h
* \brief Simplex Tree data structure for representing simplicial complexes.
Expand All @@ -91,7 +88,7 @@ struct Simplex_tree_options_full_featured;
*
*/

template<typename SimplexTreeOptions = Simplex_tree_options_full_featured>
template<typename SimplexTreeOptions = Simplex_tree_options_default>
class Simplex_tree {
public:
typedef SimplexTreeOptions Options;
Expand Down Expand Up @@ -2283,56 +2280,6 @@ std::istream& operator>>(std::istream & is, Simplex_tree<T...> & st) {
return is;
}

/** Model of SimplexTreeOptions.
*
* Maximum number of simplices to compute persistence is <CODE>std::numeric_limits<std::uint32_t>::max()</CODE>
* (about 4 billions of simplices). */
struct Simplex_tree_options_full_featured {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef double Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
};

/** Model of SimplexTreeOptions, faster than `Simplex_tree_options_full_featured` but note the unsafe
* `contiguous_vertices` option.
*
* Maximum number of simplices to compute persistence is <CODE>std::numeric_limits<std::uint32_t>::max()</CODE>
* (about 4 billions of simplices). */
struct Simplex_tree_options_fast_persistence {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef float Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = true;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
};

/** Model of SimplexTreeOptions, faster cofaces than `Simplex_tree_options_full_featured`, note the
* `link_nodes_by_label` option.
*
* Maximum number of simplices to compute persistence is <CODE>std::numeric_limits<std::uint32_t>::max()</CODE>
* (about 4 billions of simplices). */
struct Simplex_tree_options_fast_cofaces {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef double Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = true;
static const bool stable_simplex_handles = false;
};

/** @}*/ // end addtogroup simplex_tree

} // namespace Gudhi
Expand Down
93 changes: 93 additions & 0 deletions src/Simplex_tree/include/gudhi/Simplex_tree/simplex_tree_options.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/* 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): Vincent Rouvreau
*
* Copyright (C) 2023 Inria
*
* Modification(s):
* - YYYY/MM Author: Description of the modification
*/

#ifndef SIMPLEX_TREE_SIMPLEX_TREE_OPTIONS_H_
#define SIMPLEX_TREE_SIMPLEX_TREE_OPTIONS_H_

#include <gudhi/Simplex_tree/indexing_tag.h>

#include <cstdint>

namespace Gudhi {

/** \addtogroup simplex_tree
* Pre-defined options for the Simplex_tree.
* @{
*/

/** Model of SimplexTreeOptions.
*
* Default options version of the Simplex_tree. */
struct Simplex_tree_options_default {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef double Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
};

/** Model of SimplexTreeOptions.
*
* Maximum number of simplices to compute persistence is <CODE>std::numeric_limits<std::uint32_t>::max()</CODE>
* (about 4 billions of simplices). */
mglisse marked this conversation as resolved.
Show resolved Hide resolved
struct Simplex_tree_options_full_featured {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef double Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = true;
static const bool stable_simplex_handles = false;
mglisse marked this conversation as resolved.
Show resolved Hide resolved
};

/** Model of SimplexTreeOptions.
*
* Minimal version of the Simplex_tree. No filtration values are stored and it is impossible to compute persistence
* with these options. */
struct Simplex_tree_options_minimal {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef double Filtration_value;
typedef std::uint32_t Simplex_key;
mglisse marked this conversation as resolved.
Show resolved Hide resolved
static const bool store_key = false;
static const bool store_filtration = false;
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
};

/** @private @brief Model of SimplexTreeOptions, faster than `Simplex_tree_options_full_featured` but note the unsafe
VincentRouvreau marked this conversation as resolved.
Show resolved Hide resolved
* `contiguous_vertices` option.
*
* Maximum number of simplices to compute persistence is <CODE>std::numeric_limits<std::uint32_t>::max()</CODE>
* (about 4 billions of simplices). */
struct Simplex_tree_options_fast_persistence {
typedef linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef float Filtration_value;
typedef std::uint32_t Simplex_key;
static const bool store_key = true;
static const bool store_filtration = true;
static const bool contiguous_vertices = true;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
};

/** @}*/ // end addtogroup simplex_tree

} // namespace Gudhi

#endif // SIMPLEX_TREE_SIMPLEX_TREE_OPTIONS_H_
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ struct Simplex_tree_options_stable_simplex_handles {

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<Simplex_tree_options_stable_simplex_handles> > list_of_tested_variants;

template<typename Simplex_tree>
Expand Down Expand Up @@ -202,7 +202,7 @@ std::vector<std::vector<typename Simplex_tree::Vertex_handle>> get_star(Simplex_

BOOST_AUTO_TEST_CASE(simplex_fast_cofaces_rule_of_five) {
// Only for fast cofaces version to check the data structure for this feature is up to date
using STree = Simplex_tree<Simplex_tree_options_fast_cofaces>;
using STree = Simplex_tree<Simplex_tree_options_full_featured>;
STree st;

st.insert_simplex_and_subfaces({2, 1, 0}, 3.0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <gudhi/Simplex_tree.h>
#include <gudhi/Unitary_tests_utils.h>

struct Low_options : Gudhi::Simplex_tree_options_full_featured {
struct Low_options : Gudhi::Simplex_tree_options_default {
typedef float Filtration_value;
typedef std::uint8_t Vertex_handle;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ struct Simplex_tree_options_stable_simplex_handles {

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<Simplex_tree_options_stable_simplex_handles> > list_of_tested_variants;

BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_tree_expansion_all_is_blocked, typeST, list_of_tested_variants) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,22 @@

using namespace Gudhi;

struct MyOptions : Simplex_tree_options_full_featured {
struct MyOptions : Simplex_tree_options_minimal {
// Not doing persistence, so we don't need those
static const bool store_key = false;
static const bool store_filtration = false;
// I have few vertices
typedef short Vertex_handle;
};

struct MyStableOptions : Simplex_tree_options_full_featured {
struct MyStableOptions : Simplex_tree_options_default {
//disabled by default.
static const bool stable_simplex_handles = true;
};

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<MyStableOptions> > list_of_tested_variants;

BOOST_AUTO_TEST_CASE_TEMPLATE(iostream_operator, Stree_type, list_of_tested_variants) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ struct Simplex_tree_options_stable_simplex_handles {

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
mglisse marked this conversation as resolved.
Show resolved Hide resolved
Simplex_tree<Simplex_tree_options_stable_simplex_handles> > list_of_tested_variants;

BOOST_AUTO_TEST_CASE_TEMPLATE(make_filtration_non_decreasing, typeST, list_of_tested_variants) {
Expand Down
2 changes: 1 addition & 1 deletion src/Simplex_tree/test/simplex_tree_remove_unit_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

using namespace Gudhi;

struct MyOptions : Simplex_tree_options_full_featured {
struct MyOptions : Simplex_tree_options_minimal {
// Not doing persistence, so we don't need those
static const bool store_key = false;
static const bool store_filtration = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ struct Low_options : Gudhi::Simplex_tree_options_full_featured {
typedef std::uint8_t Simplex_key;
};

struct Stable_options : Gudhi::Simplex_tree_options_full_featured {
struct Stable_options : Gudhi::Simplex_tree_options_default {
//disabled by default.
static const bool stable_simplex_handles = true;
};

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Low_options>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<Stable_options> > list_of_tested_variants;

template<class Filtration_type>
Expand Down
4 changes: 2 additions & 2 deletions src/Simplex_tree/test/simplex_tree_unit_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ struct Simplex_tree_options_stable_simplex_handles {

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_persistence>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<Simplex_tree_options_stable_simplex_handles> > list_of_tested_variants;

template<class typeST>
Expand Down Expand Up @@ -1157,7 +1157,7 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_tree_boundaries_and_opposite_vertex_iterat
}

typedef boost::mpl::list<Simplex_tree<>,
Simplex_tree<Simplex_tree_options_fast_cofaces>,
Simplex_tree<Simplex_tree_options_full_featured>,
Simplex_tree<Simplex_tree_options_stable_simplex_handles> >
list_of_tested_variants_wo_fast_persistence;

Expand Down
Loading