Rework simplex tree options #978

VincentRouvreau · 2023-10-02T16:14:01Z

…ree_options_fast_persistence. Specific Simplex_tree_options_for_python for Python. Rename Simplex_tree_interface_full_featured with Simplex_tree_python_interface

…d. Some documentation fix

mglisse · 2023-10-12T19:45:46Z

src/Persistent_cohomology/example/plain_homology.cpp

 * 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;


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

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

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

Rephrase simplex tree options for this example on 0a82c1e

src/Simplex_tree/benchmark/simplex_tree_cofaces_benchmark.cpp

mglisse · 2023-10-12T19:52:50Z

src/Simplex_tree/concept/SimplexTreeOptions.h

  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 <. */


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.

src/Simplex_tree/example/mini_simplex_tree.cpp

src/Simplex_tree/include/gudhi/Simplex_tree/simplex_tree_options.h

src/Simplex_tree/test/simplex_tree_make_filtration_non_decreasing_unit_test.cpp

mglisse · 2023-10-12T20:17:49Z

src/python/include/Euclidean_witness_complex_interface.h

@@ -47,11 +47,11 @@ class Euclidean_witness_complex_interface {
    delete witness_complex_;
  }

-  void create_simplex_tree(Gudhi::Simplex_tree<>* simplex_tree, double max_alpha_square, std::size_t limit_dimension) {
+  void create_simplex_tree(Simplex_tree<Simplex_tree_options_for_python>* simplex_tree, double max_alpha_square, std::size_t limit_dimension) {


Is there a reason not to use Simplex_tree_interface here (and related places)?

Here it is ok, so I changed it on 55f9f8e
But there is an issue with Tangential_complex _interface while using create_complex. I added a comment on the problematic line

…ee_options

…me points of the code review

…mark

…ns.h Co-authored-by: Marc Glisse <[email protected]>

…eau/gudhi-devel into rework_simplex_tree_options

…ex_tree_options_for_python>

VincentRouvreau · 2023-10-19T15:13:30Z

src/python/include/Euclidean_witness_complex_interface.h

@@ -47,11 +47,11 @@ class Euclidean_witness_complex_interface {
    delete witness_complex_;
  }

-  void create_simplex_tree(Gudhi::Simplex_tree<>* simplex_tree, double max_alpha_square, std::size_t limit_dimension) {
+  void create_simplex_tree(Simplex_tree<Simplex_tree_options_for_python>* simplex_tree, double max_alpha_square, std::size_t limit_dimension) {


Here it is ok, so I changed it on 55f9f8e
But there is an issue with Tangential_complex _interface while using create_complex. I added a comment on the problematic line

VincentRouvreau · 2023-10-19T15:22:50Z

src/python/include/Tangential_complex_interface.h

-  void create_simplex_tree(Simplex_tree<>* simplex_tree) {
-    tangential_complex_->create_complex<Gudhi::Simplex_tree<Gudhi::Simplex_tree_options_full_featured>>(*simplex_tree);
+  void create_simplex_tree(Simplex_tree<Simplex_tree_options_for_python>* simplex_tree) {
+    tangential_complex_->create_complex<Simplex_tree<Simplex_tree_options_for_python>>(*simplex_tree);


Suggested change

tangential_complex_->create_complex<Simplex_tree<Simplex_tree_options_for_python>>(*simplex_tree);

tangential_complex_->create_complex<Simplex_tree_interface>(*simplex_tree);

Here, when it calls insert_simplex_and_subfaces, it calls the method from Simplex_tree_interface and it fails because it is not returning the type required by tangential complex

So the issue is that Simplex_tree_interface::insert_simplex_and_subfaces overrides the functions of the base class but has an incompatible API? Shouldn't we rename that function so it does not clash with the one in the base class then?

Actually, that function does not seem to be used anywhere, can't we remove it?

Oh, ok ! That should be some dead code. I removed it and now my suggestion is possible on a6c5e0a

…patible API that was blocking tangential_complex create_complex method

VincentRouvreau · 2023-10-20T13:32:23Z

@hschreiber @mglisse Some benchmarks to decide what options we do activate for the Python interface

link_nodes_by_label	stable_simplex_handles	OFF filename	Rips max_edge_length	Rips construction in sec	Rips construction memory in Mo	SimplexTree construction in sec	SimplexTree construction memory in Mo	SimplexTree number of simplices	Persistence computation in sec	Persistence computation memory in Mo
false	false	tore3D_300.off	3.5	0.002	1.84	1.152	1859.43	57770908	62.260	14228.86
true	false	tore3D_300.off	3.5	0.002	1.85	1.878	2741.14	57770908	67.564	14228.80
false	true	tore3D_300.off	3.5	0.002	1.81	3.412	3622.38	57770908	78.363	14228.73
true	true	tore3D_300.off	3.5	0.002	1.80	4.342	4503.83	57770908	81.205	14228.78

link_nodes_by_label	stable_simplex_handles	OFF filename	Rips construction in sec	Rips construction memory in Mo	SimplexTree construction in sec	SimplexTree construction memory in Mo	SimplexTree number of simplices	edge collapse construction in sec	edge collapse construction memory in Mo	edge collapse number of simplices	expansion(5) construction in sec	expansion(5) number of simplices	Persistence computation in sec
false	false	5-forex-2500.off	0.151	150.76	0.076	103.18	3126250	14.341	6.08	9304	0.001	20925	0.005
true	false	5-forex-2500.off	0.152	150.75	0.155	151.65	3126250	14.141	5.60	9304	0.001	20925	0.005
false	true	5-forex-2500.off	0.153	150.73	0.245	190.84	3126250	14.407	4.74	9304	0.002	20925	0.006
true	true	5-forex-2500.off	0.150	150.72	0.278	238.53	3126250	14.293	6.07	9304	0.002	20925	0.006

link_nodes_by_label	stable_simplex_handles	OFF filename	Alpha precision	Alpha construction in sec	Alpha construction memory in Mo	SimplexTree construction in sec	SimplexTree construction memory in Mo	SimplexTree number of simplices	Persistence computation in sec	Persistence computation memory in Mo
false	false	5-forex-2500.off	fast	2.041	48.97	3.364	212.66	2278347	1.615	17.99
false	false	5-forex-2500.off	safe	2.013	49.80	7.064	664.78	2278347	1.644	0.55
false	false	5-forex-2500.off	exact	1.954	49.73	23.352	720.63	2278347	1.585	14.16
true	false	5-forex-2500.off	fast	2.113	49.00	3.802	249.55	2278347	1.790	17.99
true	false	5-forex-2500.off	safe	1.831	49.76	7.330	701.88	2278347	1.925	0.55
true	false	5-forex-2500.off	exact	1.900	49.71	23.676	757.68	2278347	1.776	0.55
false	true	5-forex-2500.off	fast	2.056	48.96	4.041	278.04	2278347	1.991	17.92
false	true	5-forex-2500.off	safe	1.903	49.75	7.582	730.27	2278347	2.032	0.54
false	true	5-forex-2500.off	exact	1.834	49.78	23.621	786.18	2278347	1.966	13.43
true	true	5-forex-2500.off	fast	2.067	48.95	4.155	312.95	2278347	2.121	17.93
true	true	5-forex-2500.off	safe	1.820	49.74	7.648	765.29	2278347	2.121	0.55
true	true	5-forex-2500.off	exact	1.817	49.70	23.759	821.02	2278347	2.153	14.87

mglisse · 2023-10-20T14:12:17Z

For stable_simplex_handles, I think we said there was no point activating it in Python since for now it has 0 benefit.
For link_nodes_by_label, I think I'd be in favor of keeping it false for now (same behavior as previous releases) and possibly revisiting later. My reasoning is that it increases the size of the SimplexTree significantly (+50%), and while it makes finding cofaces less ridiculously slow on sparse complexes, that operation is still way slower than finding faces, so people who need really fast cofaces would be better off using a different datastructure that explicitly stores the links of the Hasse diagram. I am open to counter-arguments though.

hschreiber · 2023-10-23T09:50:18Z

Yes, stable_simplex_handles should be false for python, as the user never accesses the simplex handles directly, so it slows down things for nothing.
For link_nodes_by_label, I admit, I don't know how much python users need cofaces when using the simplex tree. Do we have feedback on this?

DavidLapous · 2023-10-23T15:12:17Z

A possibility, which is not headache-free, is to have both in the same simplextree : as the current python interface only stores the simplextree c++ pointer, we just need to add a private member in the python simplextree class, telling by run time how to reinterpret_cast the simplextree in c++. This may cost another python call for each method, but it should be fine. To avoid confusion of python users, it might also be useful to define __repr__ to differentiate them.

This can also apply to PR #976 .

mglisse · 2023-10-23T19:16:47Z

For link_nodes_by_label, I admit, I don't know how much python users need cofaces when using the simplex tree. Do we have feedback on this?

A search in the issues and gudhi-users finds surprisingly little. I know some people have used cofaces on examples small enough that they did not care about performance. The few cases I remember where people cared about the speed of cofaces were for C++.

A possibility, which is not headache-free, is to have both in the same simplextree : as the current python interface only stores the simplextree c++ pointer, we just need to add a private member in the python simplextree class, telling by run time how to reinterpret_cast the simplextree in c++.

So essentially a std::variant or a virtual base (though we may use other technical means to achieve it). That's a possibility. It has a cost though, both in speed (although the difference may be negligible depending on how we implement it) and maintenance (it increases and complicates the code), so it needs to be worth the trouble. But if we do decide that we want "fast" cofaces without forcing a slowdown on other operations and increasing the use of memory, that does look like a sensible approach.

For multi-persistence, while it still seems possible, my first reaction without thinking much about it is that the interfaces are likely to differ enough that a separate Python class makes more sense.

mglisse

We will need to say something in the release notes about it.

VincentRouvreau · 2023-11-20T09:15:19Z

We will need to say something in the release notes about it.

Ok, I will propose something

VincentRouvreau added 3 commits September 29, 2023 18:15

Move simplex tree options in a separated header

5326b9a

Move simplex tree options in a separated header. No doc for Simplex_t…

c0d3d1b

…ree_options_fast_persistence. Specific Simplex_tree_options_for_python for Python. Rename Simplex_tree_interface_full_featured with Simplex_tree_python_interface

Default (replacing full_feature as default), minimal and full_feature…

f9c231e

…d. Some documentation fix

mglisse reviewed Oct 12, 2023

View reviewed changes

VincentRouvreau and others added 11 commits October 16, 2023 21:36

Merge branch 'master' into rework_simplex_tree_options

99fc4dd

Merge remote-tracking branch 'upstream/master' into rework_simplex_tr…

8d2cf62

…ee_options

Fix simplex_tree_edge_expansion_unit_test after its merge

73e62bd

code review: this was not part of a code review, but inspired from so…

9c6bdc2

…me points of the code review

code review: custom simplex tree options for the cofaces access bench…

ae70563

…mark

code review: More comments

69e7956

code review: Add some details on Simplex_tree_options_default

64cebe9

code review: activate stable simplex handles on full featured options

b534bcf

Update src/Simplex_tree/include/gudhi/Simplex_tree/simplex_tree_optio…

0dea35e

…ns.h Co-authored-by: Marc Glisse <[email protected]>

Merge branch 'rework_simplex_tree_options' of github.com:VincentRouvr…

364fcd5

…eau/gudhi-devel into rework_simplex_tree_options

code review: use Simplex_tree_interface instead of Simplex_tree<Simpl…

55f9f8e

…ex_tree_options_for_python>

VincentRouvreau commented Oct 19, 2023

View reviewed changes

VincentRouvreau added 2 commits October 20, 2023 13:47

code review: rework comments about Simplex tree options

0a82c1e

code review: remove insert_simplex_and_subfaces as not used and incom…

a6c5e0a

…patible API that was blocking tangential_complex create_complex method

mglisse approved these changes Nov 18, 2023

View reviewed changes

mglisse mentioned this pull request Nov 18, 2023

multiple instantiations of Simplex_tree hidden in SimplexTree #1003

Open

VincentRouvreau merged commit ec56efe into GUDHI:master Nov 20, 2023
6 of 7 checks passed

VincentRouvreau deleted the rework_simplex_tree_options branch November 20, 2023 09:14

VincentRouvreau added the 3.9.0 GUDHI version 3.9.0 label Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework simplex tree options #978

Rework simplex tree options #978

VincentRouvreau commented Oct 2, 2023 •

edited

Loading

mglisse Oct 12, 2023

VincentRouvreau Oct 19, 2023

mglisse Oct 19, 2023

VincentRouvreau Oct 20, 2023 •

edited

Loading

mglisse Oct 12, 2023

mglisse Oct 12, 2023

VincentRouvreau Oct 19, 2023 •

edited

Loading

VincentRouvreau Oct 19, 2023 •

edited

Loading

VincentRouvreau Oct 19, 2023

mglisse Oct 19, 2023

mglisse Oct 19, 2023

VincentRouvreau Oct 20, 2023 •

edited

Loading

VincentRouvreau commented Oct 20, 2023

mglisse commented Oct 20, 2023

hschreiber commented Oct 23, 2023 •

edited

Loading

DavidLapous commented Oct 23, 2023

mglisse commented Oct 23, 2023

mglisse left a comment

VincentRouvreau commented Nov 20, 2023

	tangential_complex_->create_complex<Simplex_tree<Simplex_tree_options_for_python>>(*simplex_tree);
	tangential_complex_->create_complex<Simplex_tree_interface>(*simplex_tree);

Rework simplex tree options #978

Rework simplex tree options #978

Conversation

VincentRouvreau commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

VincentRouvreau Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

VincentRouvreau commented Oct 20, 2023

mglisse commented Oct 20, 2023

hschreiber commented Oct 23, 2023 • edited Loading

DavidLapous commented Oct 23, 2023

mglisse commented Oct 23, 2023

mglisse left a comment

Choose a reason for hiding this comment

VincentRouvreau commented Nov 20, 2023

VincentRouvreau commented Oct 2, 2023 •

edited

Loading

VincentRouvreau Oct 20, 2023 •

edited

Loading

VincentRouvreau Oct 19, 2023 •

edited

Loading

VincentRouvreau Oct 19, 2023 •

edited

Loading

VincentRouvreau Oct 20, 2023 •

edited

Loading

hschreiber commented Oct 23, 2023 •

edited

Loading