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

Multiparameter simplextree, C++ part. #976

Open
wants to merge 123 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
123 commits
Select commit Hold shift + click to select a range
60b7325
CPP part only
DavidLapous Sep 28, 2023
1fa3aa9
some tests
DavidLapous Sep 28, 2023
04d8d8c
Update src/Simplex_tree/test/simplex_tree_multi_unit_test.cpp
DavidLapous Sep 29, 2023
2cab388
Rename box.h to Box.h
DavidLapous Sep 29, 2023
cba6c6f
Rename line.h to Line.h
DavidLapous Sep 29, 2023
ddd26fe
Rename finitely_critical_filtrations.h to Finitely_critical_filtratio…
DavidLapous Sep 29, 2023
9d3b51b
copyrights
DavidLapous Sep 29, 2023
685ae13
format
DavidLapous Sep 29, 2023
1164e28
example
DavidLapous Sep 29, 2023
38c6064
typo
DavidLapous Sep 29, 2023
40fe370
Copyrights
DavidLapous Sep 29, 2023
f7965a8
include fix
DavidLapous Sep 29, 2023
c79fec6
some documentation
DavidLapous Sep 29, 2023
aab5c34
fix
DavidLapous Sep 29, 2023
6046f8d
Update src/Simplex_tree/include/gudhi/Simplex_tree/Simplex_tree_multi.h
DavidLapous Sep 29, 2023
b1e7b99
doc
DavidLapous Sep 29, 2023
fd0a492
Merge branch 'Multiparameter-Simplextree-C++-part' of github.com:Davi…
DavidLapous Sep 29, 2023
3498a8c
indentation
DavidLapous Sep 29, 2023
ebdbfda
Filtration_value concept
DavidLapous Sep 29, 2023
1433e8e
static assert setter for 1parameter sts
DavidLapous Sep 29, 2023
f4cc240
Update src/Simplex_tree/include/gudhi/Simplex_tree/multi_filtrations/…
DavidLapous Sep 29, 2023
e9668e5
find coord fix
DavidLapous Sep 29, 2023
e4bc012
doc formatting
DavidLapous Oct 3, 2023
c832b5f
typo (Thanks Vincent)
DavidLapous Oct 3, 2023
e0ae582
inf and nan limits fix part 1
DavidLapous Oct 3, 2023
37fda79
part 2 nan + inf limits for multi-filtrations
DavidLapous Oct 3, 2023
10f990d
missing import
DavidLapous Oct 3, 2023
dc2f912
Fix the auto-breakline breakages + remove python ptr interface
DavidLapous Oct 3, 2023
d61ad0b
fix numeric_limits with multi filtrations
DavidLapous Oct 3, 2023
267dd43
test fix
DavidLapous Oct 3, 2023
b85205f
.
DavidLapous Oct 3, 2023
114327f
answer to some comments
DavidLapous Oct 3, 2023
04ce9e2
doc improvements
DavidLapous Oct 4, 2023
3f4dc40
doc improvements
DavidLapous Oct 4, 2023
d070bf1
set number of parameter in multify
DavidLapous Oct 4, 2023
96056a3
more tests
DavidLapous Oct 4, 2023
6d5ad8b
Optional num_parameters, vincent's tests of simplextree multi
DavidLapous Oct 5, 2023
a89ff98
fix make_filtration_non_decreasing
DavidLapous Oct 5, 2023
a6d756a
edge cases comparison fix
DavidLapous Oct 11, 2023
6fa20cf
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Oct 11, 2023
f90cce7
doc update
DavidLapous Oct 13, 2023
f03df17
merge
DavidLapous Oct 13, 2023
bc58007
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Oct 19, 2023
dcdd710
missing is_multi_parameter in options
DavidLapous Oct 19, 2023
e5123d6
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Nov 15, 2023
26d78ab
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Nov 17, 2023
1c0e1df
Merge branch 'master' into Multiparameter-Simplextree-C++-part
DavidLapous Nov 21, 2023
0ef649f
multiparameter flag
DavidLapous Nov 21, 2023
b448c05
multiparameter python flag
DavidLapous Nov 21, 2023
89dfd6b
multiparameter flags
DavidLapous Nov 21, 2023
cf41588
example fix
DavidLapous Nov 21, 2023
4ecdd32
fix test
DavidLapous Nov 21, 2023
49ee642
Merge branch 'Multiparameter-Simplextree-C++-part' of github.com:Davi…
DavidLapous Nov 21, 2023
9de0252
merge
DavidLapous Jun 23, 2024
5b7e276
update
DavidLapous Jun 23, 2024
8c797f2
fix: test & tbb
DavidLapous Jun 24, 2024
8b303c2
fix: include missing in stmulti exemple
DavidLapous Jun 25, 2024
27ebaf6
fix: unnecessary tbb include
DavidLapous Jun 25, 2024
c7cd3d3
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jun 26, 2024
214ebc6
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jun 29, 2024
afde822
Update src/Simplex_tree/include/gudhi/Simplex_tree/multi_filtrations/…
DavidLapous Jul 1, 2024
039415d
fix: remove unnecessary multiparameter flag
DavidLapous Jul 1, 2024
6c27892
fix: inf_ simplextree static member should be const
DavidLapous Jul 1, 2024
882bf8c
feat: Assert in the line constructor that the arguments are correct.
DavidLapous Jul 1, 2024
a15d72c
fix: doc missing and not correct infinite values.
DavidLapous Jul 1, 2024
460e1a0
doc: added some doc for the box class
DavidLapous Jul 1, 2024
9b7612e
doc: added a bit of doc in the multi-filtrations file
DavidLapous Jul 1, 2024
e5eead4
fix: typo in doc
DavidLapous Jul 1, 2024
3344c17
revert: remove the const to inf_ (conflicts with filtration_mutable)
DavidLapous Jul 2, 2024
1e8fc4f
doc: small update
DavidLapous Jul 2, 2024
03cf667
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jul 6, 2024
6be37e7
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jul 12, 2024
21d95cb
Update src/Simplex_tree/example/simplex_tree_multi.cpp
DavidLapous Jul 16, 2024
6338fc7
doc
DavidLapous Jul 16, 2024
3d480b6
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jul 18, 2024
8eac4b9
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Jul 31, 2024
ac575db
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Aug 2, 2024
065a8b9
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Aug 12, 2024
959a3d3
Move and rename in 2 new modules Multi_filtration and Multi_persistence
VincentRouvreau Aug 19, 2024
a42608e
Bad copy/paste from editor
VincentRouvreau Aug 20, 2024
893fbf6
Move Line and Box in a Multi_persistence folder to enhance inclusion …
VincentRouvreau Aug 20, 2024
ffaf004
Add example in doc
VincentRouvreau Aug 20, 2024
be3ba48
clang-format moved files
VincentRouvreau Aug 20, 2024
30397ba
Failed clang-format
VincentRouvreau Aug 20, 2024
01d4a30
Merge pull request #1 from VincentRouvreau/multi_parameter_module
DavidLapous Aug 22, 2024
62bd472
one_critical_filtration
hschreiber Aug 23, 2024
4d6dbbf
Merge pull request #2 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Aug 26, 2024
6d7d5f6
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Aug 27, 2024
a882194
multi_critical_filtration
hschreiber Aug 27, 2024
777a766
Merge branch 'GUDHI:master' into multiparam_st_schreib_clean
hschreiber Aug 27, 2024
3ae5312
removal of get_copy
hschreiber Aug 27, 2024
92ce37c
removal of get-content and renaming of fil
hschreiber Aug 29, 2024
ff4d970
Merge pull request #3 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Aug 29, 2024
9258390
doc + renaming
hschreiber Aug 29, 2024
7d66c00
re-format
hschreiber Aug 29, 2024
5a80760
add enum class for _get_domination_relation
hschreiber Aug 29, 2024
59f99b0
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Aug 30, 2024
15324a3
windows fix
hschreiber Aug 30, 2024
d3625c4
correction doc + no empty values for multicritical
hschreiber Sep 2, 2024
ebafc31
Merge pull request #4 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Sep 3, 2024
bac48dc
unit test fix
hschreiber Sep 3, 2024
bc64d15
Merge remote-tracking branch 'origin/multiparam_st_schreib_clean' int…
hschreiber Sep 3, 2024
8385ac4
windows fix
hschreiber Sep 3, 2024
00ad5a5
Merge remote-tracking branch 'origin/multiparam_st_schreib_clean' int…
hschreiber Sep 3, 2024
361134a
windows fix
hschreiber Sep 3, 2024
76390f4
Merge remote-tracking branch 'origin/multiparam_st_schreib_clean' int…
hschreiber Sep 3, 2024
4190ed7
doc line and box
hschreiber Sep 4, 2024
19aba94
unit tests for box and line
hschreiber Sep 4, 2024
69986c3
Merge pull request #5 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Sep 5, 2024
7925e87
add sort in multicritical + other small fixes
hschreiber Sep 5, 2024
c5263dc
doc
hschreiber Sep 5, 2024
732c2c3
co fix
hschreiber Sep 5, 2024
1e856ec
Merge pull request #6 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Sep 6, 2024
0dd084c
minor fixes
hschreiber Sep 6, 2024
d2a2168
Merge branch 'GUDHI:master' into multiparam_st_schreib_clean
hschreiber Sep 6, 2024
9ec506a
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Sep 12, 2024
02cb463
small fixes
hschreiber Sep 24, 2024
f6d5b7e
merge upstream
hschreiber Nov 15, 2024
17d1caa
Merge branch 'Multiparameter-Simplextree-C++-part' into multiparam_st…
hschreiber Nov 15, 2024
b752394
merge fix
hschreiber Nov 15, 2024
1b24266
Merge pull request #7 from DavidLapous/multiparam_st_schreib_clean
DavidLapous Nov 21, 2024
bf9ecf6
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Nov 21, 2024
d4b4e19
Merge branch 'GUDHI:master' into Multiparameter-Simplextree-C++-part
DavidLapous Dec 12, 2024
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
2 changes: 2 additions & 0 deletions src/Persistent_cohomology/test/betti_numbers_unit_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ struct MiniSTOptions : Gudhi::Simplex_tree_options_full_featured {
static const bool store_key = true;
// I have few vertices
typedef short Vertex_handle;

static const bool is_multi_parameter = 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 think I already commented somewhere that we should remove this line (and the same in several places) because is_multi_parameter is already false in the base class.

};

using Mini_simplex_tree = Gudhi::Simplex_tree<MiniSTOptions>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ struct MiniSTOptions {
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
static const bool is_multi_parameter = false;
};

using Mini_simplex_tree = Gudhi::Simplex_tree<MiniSTOptions>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ struct Simplex_tree_options_stable_simplex_handles {
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = true;
static const bool stable_simplex_handles = true;
static const bool is_multi_parameter = false;
};

int main(int argc, char *argv[]) {
Expand Down
2 changes: 2 additions & 0 deletions src/Simplex_tree/concept/SimplexTreeOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,7 @@ struct SimplexTreeOptions {
static const bool link_nodes_by_label;
/// If true, Simplex_handle will not be invalidated after insertions or removals.
static const bool stable_simplex_handles;
/// If true, assumes that Filtration_value is vector-like instead of float-like. This also assumes that Filtration_values is a class, which has a push_to method to push a filtration value $x$ onto $this>=0$.
DavidLapous marked this conversation as resolved.
Show resolved Hide resolved
static const bool is_multi_parameter;
};

7 changes: 7 additions & 0 deletions src/Simplex_tree/example/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ if(TARGET TBB::tbb)
endif()
add_test(NAME Simplex_tree_example_mini_simplex_tree COMMAND $<TARGET_FILE:Simplex_tree_example_mini_simplex_tree>)

add_executable ( Simplex_tree_multi_example simplex_tree_multi.cpp )
if(TARGET TBB::tbb)
target_link_libraries(Simplex_tree_multi_example TBB::tbb)
endif()
add_test(NAME Simplex_tree_multi_example COMMAND $<TARGET_FILE:Simplex_tree_multi_example>)


# An example with Simplex-tree using CGAL alpha_shapes_3
if(GMP_FOUND AND NOT CGAL_VERSION VERSION_LESS 4.11.0)
add_executable ( Simplex_tree_example_alpha_shapes_3_from_off example_alpha_shapes_3_simplex_tree_from_off_file.cpp )
Expand Down
56 changes: 56 additions & 0 deletions src/Simplex_tree/example/simplex_tree_multi.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/* 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): David Loiseaux
*
* Copyright (C) 2023 Inria
*
* Modification(s):
* - YYYY/MM Author: Description of the modification
*/

#include <gudhi/Simplex_tree.h>
#include <gudhi/Simplex_tree/Simplex_tree_multi.h>

#include <iostream>
#include <initializer_list>

struct ST_MULTI {
public:
DavidLapous marked this conversation as resolved.
Show resolved Hide resolved
typedef Gudhi::linear_indexing_tag Indexing_tag;
typedef int Vertex_handle;
typedef float value_type;
using Filtration_value = Gudhi::multiparameter::multi_filtrations::Finitely_critical_multi_filtration<value_type>;
Copy link
Member

Choose a reason for hiding this comment

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

The nested multiparameter::multi_filtrations feel a bit redundant, but I guess it isn't that bad.

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;
static const bool is_multi_parameter = true;
};

using ST = Gudhi::Simplex_tree<ST_MULTI>;


int main() {
ST st;

/* Complex to build. */
/* 1 */
/* o */
/* /X\ */
/* o---o---o */
/* 2 0 3 */

auto triangle012 = {0, 1, 2};
auto edge03 = {0, 3};
st.insert_simplex_and_subfaces(triangle012, {1,2,3}); // {1,2,3} can be any array-like vector-like
st.insert_simplex_and_subfaces(edge03, {4,5,6});

auto edge02 = {0, 2};
ST::Simplex_handle e = st.find(edge02);
// Finitely_critical_multi_filtration has an operator<<
std::cout << st.filtration(e) << std::endl;
assert(st.filtration(st.find(edge03)) == std::vector<float>({4,5,6}));
DavidLapous marked this conversation as resolved.
Show resolved Hide resolved

}
107 changes: 76 additions & 31 deletions src/Simplex_tree/include/gudhi/Simplex_tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,18 @@ class Simplex_tree {
Key_simplex_base;

struct Filtration_simplex_base_real {
Filtration_simplex_base_real() : filt_(0) {}
void assign_filtration(Filtration_value f) { filt_ = f; }
Filtration_value filtration() const { return filt_; }
Filtration_simplex_base_real() : filt_{} {}
void assign_filtration(const Filtration_value& f) { filt_ = f; }
const Filtration_value& filtration() const { return filt_; }
Filtration_value& filtration() { return filt_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

TBD, but I am not sure we should add this method.
And if we add it we should add it to the dummy one.

private:
Filtration_value filt_;
};
struct Filtration_simplex_base_dummy {
Filtration_simplex_base_dummy() {}
void assign_filtration(Filtration_value GUDHI_CHECK_code(f)) { GUDHI_CHECK(f == 0, "filtration value specified for a complex that does not store them"); }
Copy link
Member

Choose a reason for hiding this comment

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

Does this test f==0 do something sensible for multifiltration? Hmm, failing to compile may actually be a sensible behavior here, ok.

Filtration_value filtration() const { return 0; }
const Filtration_value& filtration() const { return null_value; }
static constexpr Filtration_value null_value={};
Copy link
Contributor

@VincentRouvreau VincentRouvreau Sep 29, 2023

Choose a reason for hiding this comment

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

If we say:

struct MyOptions : Simplex_tree_options_multidimensional_filtration {
  // is_multi_parameter but not store_filtration
  static const bool store_filtration = false;
};

Simplex_tree<MyOptions> stree;

We are facing the error:

gudhi-devel/src/Simplex_tree/include/gudhi/Simplex_tree.h:165:39: error: the type 'const Filtration_value' {aka 'const Gudhi::multiparameter::multi_filtrations::Finitely_critical_multi_filtration<float>'} of 'constexpr' variable 'Gudhi::Simplex_tree<MyOptions>::Filtration_simplex_base_dummy::null_value' is not literal
     static constexpr Filtration_value null_value={};
                                       ^~~~~~~~~~

But maybe this error is normal ? I was wondering to see if we should static_assert some error in this weird case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A static_assert could be a good idea, as the multi parameter tree only differentiate it self from the standard with its filtration values. If store_filtration == false, is_multi_parameter set to true makes no sense. But the problem could also be solved by removing null_value (see Issue #977), then we can just document that is_multi_parameter is ignored if store_filtration == false.

Copy link
Member Author

@DavidLapous DavidLapous Oct 3, 2023

Choose a reason for hiding this comment

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

A simple fix is to do a static_assert(!store_filtration || is_multi_parameter). thoughts ?

Copy link
Member

Choose a reason for hiding this comment

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

I guess you meant static_assert(store_filtration || !is_multi_parameter) instead?

};
typedef typename std::conditional<Options::store_filtration, Filtration_simplex_base_real,
Filtration_simplex_base_dummy>::type Filtration_simplex_base;
Expand Down Expand Up @@ -576,7 +578,7 @@ class Simplex_tree {
*
* Same as `filtration()`, but does not handle `null_simplex()`.
*/
static Filtration_value filtration_(Simplex_handle sh) {
static const Filtration_value& filtration_(Simplex_handle sh) {
GUDHI_CHECK (sh != null_simplex(), "null simplex");
return sh->second.filtration();
}
Expand Down Expand Up @@ -604,18 +606,25 @@ class Simplex_tree {
* Called on the null_simplex, it returns infinity.
* If SimplexTreeOptions::store_filtration is false, returns 0.
*/
static Filtration_value filtration(Simplex_handle sh) {
static const Filtration_value& filtration(Simplex_handle sh){
if (sh != null_simplex()) {
return sh->second.filtration();
} else {
return std::numeric_limits<Filtration_value>::infinity();
return inf_;
}
}
static Filtration_value& filtration_mutable(Simplex_handle sh){
if (sh != null_simplex()) {
return sh->second.filtration();
} else {
return inf_;
Comment on lines +690 to +701
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already from the previous PR:

I am wondering if we could not just keep the version with the non const, as the validity of the filtration in the simplex tree is already responsibility of the user. This would avoid having to find a different name when it does exactly the same thing than filtration(). Or could there be performance issues when using non const references for numerical values (i.e., when the simplex tree is not multi)? If it is the case, an ugly way to solve this would be to return a std::conditional to switch between Filtration_value& and Filtration_value depending on the value of is_multi_parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a fixed point of view on this. So its only a matter of what's the gudhi style of accessing a filtration value by reference.

Copy link
Member

Choose a reason for hiding this comment

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

If they really do the same thing, I am ok with filtration returning a non-const reference. But I wonder if filtration_mutable actually needs to return infinity for the null simplex. If we decided instead that filtration_mutable only accepts real simplices (GUDHI_CHECK that sh!=null_simplex()), it could make sense for it to be a separate function, and it would let us declare inf_ const.
IIRC (that was a while ago, so I am likely wrong), handling null_simplex was mostly for convenience for the output of persistent cohomology, not some fundamental reason.

}
}

/** \brief Sets the filtration value of a simplex.
* \exception std::invalid_argument In debug mode, if sh is a null_simplex.
*/
void assign_filtration(Simplex_handle sh, Filtration_value fv) {
void assign_filtration(Simplex_handle sh, const Filtration_value& fv) {
GUDHI_CHECK(sh != null_simplex(),
std::invalid_argument("Simplex_tree::assign_filtration - cannot assign filtration on null_simplex"));
sh->second.assign_filtration(fv);
Expand Down Expand Up @@ -822,14 +831,16 @@ class Simplex_tree {
* to the new simplex.
* If the insertion fails (the simplex is already there), the bool is set to false. If the insertion
* fails and the simplex already in the complex has a filtration value strictly bigger than 'filtration',
* and the simplex tree is not multi parameter (`SimplexTreeOptions::is_multi_parameter == false`),
* 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.
* output pair to the Simplex_handle of the simplex. When the simplex tree is multi parameter,
* the existing filtration values are not updated. If the insertion fails for other reasons,
Comment on lines +933 to +934
Copy link
Member

Choose a reason for hiding this comment

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

  1. Does this comment only apply to this function, or also the other insertions as well?
  2. What is the rationale for not updating the value? For multi_critical, "min" is replaced by adding a critical value (and possibly removing redundant ones). For one-critical, if the insertion would turn it into multi-critical, we could throw an exception. Basically, I am wondering in what cases we are going to use insert on a simplex that may be already there and we will want it to keep its old filtration value. Is it for cases where we want to insert without caring about the filtration value, and we will manually set the filtration value later?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I think this applies to every insertion; the code was meant to work with the python interface, so I may have missed c++ only stuff.
  2. I think throwing an exception is a good idea for the one-critical one. For the multi-critical one, IIRC there were non-trivial changes to do to the current simplextree cpp insertion, and I had to cheat my way out of it: see this python interface code (warning, this is ugly). Is there an easy way to make (every) insertions work when a simplex is already there in the k-critical setting ? It's been a while since I've touched at this code.

* we set the Simplex_handle part to `null_simplex`.
*
*/
template <class RandomVertexHandleRange = std::initializer_list<Vertex_handle>>
std::pair<Simplex_handle, bool> insert_simplex_raw(const RandomVertexHandleRange& simplex,
Filtration_value filtration) {
const Filtration_value& filtration) {
Siblings * curr_sib = &root_;
std::pair<Simplex_handle, bool> res_insert;
auto vi = simplex.begin();
Expand Down Expand Up @@ -895,7 +906,7 @@ class Simplex_tree {
* .end() return input iterators, with 'value_type' Vertex_handle. */
template<class InputVertexRange = std::initializer_list<Vertex_handle>>
std::pair<Simplex_handle, bool> insert_simplex(const InputVertexRange & simplex,
Filtration_value filtration = 0) {
const Filtration_value& filtration = {}) {
auto first = std::begin(simplex);
auto last = std::end(simplex);

Expand Down Expand Up @@ -924,7 +935,7 @@ class Simplex_tree {
*/
template<class InputVertexRange = std::initializer_list<Vertex_handle>>
std::pair<Simplex_handle, bool> insert_simplex_and_subfaces(const InputVertexRange& Nsimplex,
Filtration_value filtration = 0) {
VincentRouvreau marked this conversation as resolved.
Show resolved Hide resolved
const Filtration_value& filtration = {}) {
auto first = std::begin(Nsimplex);
auto last = std::end(Nsimplex);

Expand Down Expand Up @@ -953,7 +964,7 @@ class Simplex_tree {
std::pair<Simplex_handle, bool> rec_insert_simplex_and_subfaces_sorted(Siblings* sib,
ForwardVertexIterator first,
ForwardVertexIterator last,
Filtration_value filt) {
const Filtration_value& filt) {
// An alternative strategy would be:
// - try to find the complete simplex, if found (and low filtration) exit
// - insert all the vertices at once in sib
Expand All @@ -969,14 +980,17 @@ class Simplex_tree {

Simplex_handle simplex_one = insertion_result.first;
bool one_is_new = insertion_result.second;
if (!one_is_new) {
if (filtration(simplex_one) > filt) {
assign_filtration(simplex_one, filt);
} else {
// FIXME: this interface makes no sense, and it doesn't seem to be tested.
insertion_result.first = null_simplex();
if constexpr (!SimplexTreeOptions::is_multi_parameter){ // Ignores the assign part for multiparameter filtrations.
if (!one_is_new) {
if (filtration(simplex_one) > filt){
assign_filtration(simplex_one, filt);
} else {
// FIXME: this interface makes no sense, and it doesn't seem to be tested.
insertion_result.first = null_simplex();
}
}
}

if (++first == last) return insertion_result;
if (!has_children(simplex_one))
// TODO: have special code here, we know we are building the whole subtree from scratch.
Expand Down Expand Up @@ -1316,7 +1330,7 @@ class Simplex_tree {
* The complex does not need to be empty before calling this function. However, if a vertex is
* already present, its filtration value is not modified, unlike with other insertion functions. */
template <class VertexRange>
void insert_batch_vertices(VertexRange const& vertices, Filtration_value filt = 0) {
void insert_batch_vertices(VertexRange const& vertices, const Filtration_value& filt ={}) {
auto verts = vertices | boost::adaptors::transformed([&](auto v){
return Dit_value_t(v, Node(&root_, filt)); });
root_.members_.insert(boost::begin(verts), boost::end(verts));
Expand Down Expand Up @@ -1403,7 +1417,7 @@ class Simplex_tree {
static void intersection(std::vector<std::pair<Vertex_handle, Node> >& intersection,
Dictionary_it begin1, Dictionary_it end1,
Dictionary_it begin2, Dictionary_it end2,
Filtration_value filtration_) {
const Filtration_value& filtration_) {
if (begin1 == end1 || begin2 == end2)
return; // ----->>
while (true) {
Expand Down Expand Up @@ -1610,12 +1624,21 @@ class Simplex_tree {
if (dim == 0) return;
// Find the maximum filtration value in the border
Boundary_simplex_range&& boundary = boundary_simplex_range(sh);
Boundary_simplex_iterator max_border = std::max_element(std::begin(boundary), std::end(boundary),
[](Simplex_handle sh1, Simplex_handle sh2) {
return filtration(sh1) < filtration(sh2);
});
Filtration_value max_filt_border_value;
if constexpr (SimplexTreeOptions::is_multi_parameter) {
// in that case, we assume that Filtration_value has a `push_to` member to handle this.
VincentRouvreau marked this conversation as resolved.
Show resolved Hide resolved
max_filt_border_value = Filtration_value(this->number_of_parameters_);
for (auto& face_sh : boundary) {
max_filt_border_value.push_to(
filtration(face_sh)); // pushes the value of max_filt_border_value to reach simplex' filtration
}
} else {
Boundary_simplex_iterator max_border =
std::max_element(std::begin(boundary), std::end(boundary),
[](Simplex_handle sh1, Simplex_handle sh2) { return filtration(sh1) < filtration(sh2); });
max_filt_border_value = filtration(*max_border);
}

Filtration_value max_filt_border_value = filtration(*max_border);
// Replacing if(f<max) with if(!(f>=max)) would mean that if f is NaN, we replace it with the max of the children.
// That seems more useful than keeping NaN.
if (!(sh->second.filtration() >= max_filt_border_value)) {
Expand Down Expand Up @@ -1650,7 +1673,7 @@ class Simplex_tree {
* than it was before. However, `upper_bound_dimension()` will return the old value, which remains a valid upper
* bound. If you care, you can call `dimension()` to recompute the exact dimension.
*/
bool prune_above_filtration(Filtration_value filtration) {
bool prune_above_filtration(const Filtration_value& filtration) {
if (std::numeric_limits<Filtration_value>::has_infinity && filtration == std::numeric_limits<Filtration_value>::infinity())
return false; // ---->>
bool modified = rec_prune_above_filtration(root(), filtration);
Expand All @@ -1660,7 +1683,7 @@ class Simplex_tree {
}

private:
bool rec_prune_above_filtration(Siblings* sib, Filtration_value filt) {
bool rec_prune_above_filtration(Siblings* sib, const Filtration_value& filt) {
auto&& list = sib->members();
auto last = std::remove_if(list.begin(), list.end(), [this,filt](Dit_value_t& simplex) {
if (simplex.second.filtration() <= filt) return false;
Expand Down Expand Up @@ -2070,7 +2093,7 @@ class Simplex_tree {
* @param[in] filt_value The new filtration value.
* @param[in] min_dim The minimal dimension. Default value is 0.
*/
void reset_filtration(Filtration_value filt_value, int min_dim = 0) {
void reset_filtration(const Filtration_value& filt_value, int min_dim = 0) {
rec_reset_filtration(&root_, filt_value, min_dim);
clear_filtration(); // Drop the cache.
}
Expand All @@ -2081,7 +2104,7 @@ class Simplex_tree {
* @param[in] filt_value The new filtration value.
* @param[in] min_depth The minimal depth.
*/
void rec_reset_filtration(Siblings * sib, Filtration_value filt_value, int min_depth) {
void rec_reset_filtration(Siblings * sib, const Filtration_value& filt_value, int min_depth) {
for (auto sh = sib->members().begin(); sh != sib->members().end(); ++sh) {
if (min_depth <= 0) {
sh->second.assign_filtration(filt_value);
Expand Down Expand Up @@ -2246,6 +2269,24 @@ class Simplex_tree {
/** \brief Upper bound on the dimension of the simplicial complex.*/
int dimension_;
bool dimension_to_be_lowered_ = false;

// MULTIPERS STUFF
public:
/**
* \brief Sets the number of parameters of the filtrations if SimplexTreeOptions::is_multi_parameter.
* */
void set_number_of_parameters(int num) { number_of_parameters_ = num; }
VincentRouvreau marked this conversation as resolved.
Show resolved Hide resolved
/**
* \brief Gets the number of parameters of the filtrations if SimplexTreeOptions::is_multi_parameter.
* */
int get_number_of_parameters() const { return number_of_parameters_; }

inline static Filtration_value inf_ = std::numeric_limits<Filtration_value>::has_infinity ?
Copy link
Member

Choose a reason for hiding this comment

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

const?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an issue with filtration_mutable which has to return a reference, I don't really know what's the best way to deal with this

std::numeric_limits<Filtration_value>::infinity()
: std::numeric_limits<Filtration_value>::max(); /**< Default infinite value. */

private:
int number_of_parameters_; /**< Number of parameters of the multi-filtrations when SimplexTreeOptions::is_multi_parameter.-*/
Copy link
Contributor

Choose a reason for hiding this comment

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

number_of_parameters_ default value is 0. TBD, but wouldn't it be clearer from a user point of view to set it to 2 by default ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, why not, as the get number of parameters will always be 1 for non-multiparam simplextrees, this default value seems good.

Copy link
Member

Choose a reason for hiding this comment

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

OTOH, a "broken" value like 0 (or even -1) forces the user to be explicit (which can be seen as good or bad).

};

// Print a Simplex_tree in os.
Expand Down Expand Up @@ -2297,6 +2338,7 @@ struct Simplex_tree_options_full_featured {
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
static const bool is_multi_parameter = false;
};

/** Model of SimplexTreeOptions, faster than `Simplex_tree_options_full_featured` but note the unsafe
Expand All @@ -2314,6 +2356,7 @@ struct Simplex_tree_options_fast_persistence {
static const bool contiguous_vertices = true;
static const bool link_nodes_by_label = false;
static const bool stable_simplex_handles = false;
static const bool is_multi_parameter = false;
};

/** Model of SimplexTreeOptions, faster cofaces than `Simplex_tree_options_full_featured`, note the
Expand All @@ -2331,6 +2374,8 @@ struct Simplex_tree_options_fast_cofaces {
static const bool contiguous_vertices = false;
static const bool link_nodes_by_label = true;
static const bool stable_simplex_handles = false;
static const bool is_multi_parameter = false;

};

/** @}*/ // end addtogroup simplex_tree
Expand Down
Loading
Loading