Const simplex tree unitary tests and fix operator== and != #1145

VincentRouvreau · 2024-10-18T09:36:08Z

This PR is based on and replaces #1119

…l into const_simplex_tree

hschreiber · 2024-10-18T09:44:25Z

Thanks for the unitary tests! But I think you forgot to include them?

VincentRouvreau · 2024-10-18T10:20:00Z

Thanks for the unitary tests! But I think you forgot to include them?

Oops yes. Should be better now

hschreiber · 2024-10-18T12:44:11Z

Should we update the credit banners with the changes or is it not really necessary? Otherwise, looks good to me.

mglisse

Looks ok. There may be things we can clean-up later, but they don't seem like prerequisites.
The change will have to be mentioned in next_release.md (we can now perform many operations on a const ST, but we are now stricter about undocumented direct use of Simplex_handle).

mglisse · 2024-10-18T16:19:22Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -715,7 +719,7 @@ class Simplex_tree {
  static Simplex_data& simplex_data(Simplex_handle sh) {


Should we make it non-static, with const/non-const overloads?

Done and tested on 99b37a3

src/Simplex_tree/include/gudhi/Simplex_tree.h

mglisse · 2024-10-18T16:27:00Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -658,6 +658,10 @@ class Simplex_tree {
    return sh->second.filtration();
  }

+  static Dictionary_it _to_node_it(Simplex_handle sh){


It feels like making this a non-const member function instead of a static one would be safer, but that's an implementation detail, it is ok as static.

No more static on 85efd97
I also added a comment on that method

src/Simplex_tree/include/gudhi/Simplex_tree.h

mglisse · 2024-10-18T16:46:28Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -1987,7 +1998,7 @@ class Simplex_tree {
      if (!(sh->second.filtration() >= max_filt_border_value)) {
        // Store the filtration modification information
        modified = true;
-        sh->second.assign_filtration(max_filt_border_value);
+        _to_node_it(sh)->second.assign_filtration(max_filt_border_value);


It is ok for now, but in the long run, internally, do we want to use _to_node_it or do we want to go through the official interface assign_filtration(sh, max_filt_border_value)? _to_node_it has the advantage (?) of skipping the assertion, but the code is uglier.

You are right, we should prefer the use of assign_filtration(sh, max_filt_border_value). I modified it on 0d500bc

mglisse · 2024-10-18T16:51:01Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -2374,10 +2386,21 @@ class Simplex_tree {
    }
    return nullptr;
  }
+  List_max_vertex const* nodes_by_label(Vertex_handle v) const {


I think you could have the other function call this one and const_cast the result, to avoid duplicating the code.

I did so on f8de11a but I did not find out how to do it without an indirection

std::as_const(*this).nodes_by_label(v)?

Oh ? Ok ! I did return const_cast<List_max_vertex*>(std::as_const(*this).nodes_by_label(v)); on 4277a64

mglisse · 2024-10-26T12:59:29Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

+  }
+
+  /** \brief Returns the extra data stored in a simplex. */
+  const Simplex_data simplex_data(Simplex_handle sh) const {


Suggested change

const Simplex_data simplex_data(Simplex_handle sh) const {

const Simplex_data& simplex_data(Simplex_handle sh) const {

?

You are right, done on cbbdc8a

mglisse · 2024-10-26T13:03:47Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -2374,10 +2386,21 @@ class Simplex_tree {
    }
    return nullptr;
  }
+  List_max_vertex const* nodes_by_label(Vertex_handle v) const {


std::as_const(*this).nodes_by_label(v)?

mglisse · 2024-10-26T13:36:30Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

    assert(dimension(sh) == 1);
    return { find_vertex(sh->first), find_vertex(self_siblings(sh)->parent()) };
  }

  /** Returns the Siblings containing a simplex.*/
  template<class SimplexHandle>
-  static Siblings* self_siblings(SimplexHandle sh) {
+  static Siblings const* self_siblings(SimplexHandle sh) {


It looks like having const and non-const overloads of self_siblings would avoid a couple const_cast elsewhere.

I did so on 85efd97 and it is true that it removes some const_cast

mglisse · 2024-10-26T13:44:02Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -1129,7 +1143,7 @@ class Simplex_tree {
   *
   * Any insertion, deletion or change of filtration value invalidates this cache,
   * which can be cleared with clear_filtration().  */
-  void initialize_filtration(bool ignore_infinite_values = false) {
+  void initialize_filtration(bool ignore_infinite_values = false) const {


Should clear_filtration also be const, or is it different enough that it shouldn't be?

clear_filtration() constify on 3d21b0c

mglisse · 2024-10-26T14:15:52Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

+  // all mutable as their content has no impact on the content of the simplex tree itself
+  // they correspond to some kind of cache or helper attributes.


At some point we should document what functions are safe to use in parallel on a Simplex_tree and what functions are not. Some people have the expectation that const means thread-safe, which isn't the case in the presence of mutable.

I marked dimension(), upper_bound_dimension(), initialize_filtration(), maybe_initialize_filtration (), clear_filtration(), filtration_simplex_range(), simplex(Simplex_key idx), print_hasse() as not thread-safe in the doc on 3d21b0c
Remark, that print_hasse() should not and use complex_simplex_range() instead of filtration_simplex_range().

… thread-safe

hschreiber · 2024-11-07T12:19:49Z

src/Simplex_tree/test/simplex_tree_unit_test.cpp

  BOOST_CHECK(STRoot != nullptr);
  BOOST_CHECK(STRoot->oncles() == nullptr);
  BOOST_CHECK(STRoot->parent() == DEFAULT_VERTEX_VALUE);
  BOOST_CHECK(tst.dimension() == -1);
 }

 template<class typeST>
-void test_iterators_on_empty_simplex_tree(typeST& tst) {
+void test_iterators_on_empty_simplex_tree(const typeST& tst) {
  std::clog << "Iterator on vertices: " << std::endl;
  for (auto vertex : tst.complex_vertex_range()) {


It looks like this is the only place where complex_vertex_range is tested. Could be nice to test it also for non empty trees?

I added a test on 38a751f

hschreiber · 2024-11-07T12:32:49Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -671,6 +679,8 @@ class Simplex_tree {
  /** \brief Returns the simplex that has index idx in the filtration.
   *
   * The filtration must be initialized.
+   *
+   * @note Not thread safe
   */
  Simplex_handle simplex(Simplex_key idx) const {


I couldn't find any place where this method is tested, is it const or non const. I guess, if filtration_simplex_range works, this method should work too.

Same for contiguous_vertices().

I added 2 new tests on 38a751f

hschreiber · 2024-11-07T12:39:39Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -827,7 +849,7 @@ class Simplex_tree {
  /** \brief Returns the children of the node in the simplex tree pointed by sh.
   * \exception std::invalid_argument In debug mode, if sh has no child.
   */
-  Siblings* children(Simplex_handle sh) const {
+  Siblings const* children(Simplex_handle sh) const {
    GUDHI_CHECK(has_children(sh), std::invalid_argument("Simplex_tree::children - argument has no child"));


This is not directly related to this PR, but is there a reason why has_children (just above at line 840, I can't select it) is public? Or even root?

I added some const test about has_children() and root() on 323e119

hschreiber · 2024-11-07T12:59:06Z

src/Simplex_tree/include/gudhi/Simplex_tree.h

@@ -1934,7 +1971,7 @@ class Simplex_tree {
   * the children of this simplex (a subset of the cofaces).
   */
  template<class Fun>
-  void for_each_simplex(Fun&& fun) {
+  void for_each_simplex(Fun&& fun) const {


We should also test the method on a non const simplex tree such that fun modified something in the tree (eg. puts all filtration values to value 2), to ensure that the constness of the method does not block that. I think we already test it indirectly with some use of for_each_simplex in another place, but I suddenly have a doubt.

Not tested directly, but this is what is done on assign_MEB_filtration (cf. test_cech_complex, delaunay_cech_persistence utils)

…lex_vertex_range

…static with a const/non const version if necessary

hschreiber and others added 12 commits August 2, 2024 19:43

begining

250757d

const compiles

5ac7d58

Merge branch 'GUDHI:master' into const_simplex_tree

f5f9922

merge upstream + doc

587c03d

Merge branch 'GUDHI:master' into const_simplex_tree

ef6c96c

upstream merge

a5e3e0f

Merge branch 'const_simplex_tree' of github.com:hschreiber/gudhi-deve…

0820a27

…l into const_simplex_tree

upstream merge

b7f4649

restore Persistent_cohomology.h

e425049

removal of non const ranges

8067ba7

removal of non const ranges

06834b9

Fix operator== and != for constness. Add some TU for constness

8055160

New unitary test for simplex tree constness

f8b8912

mglisse reviewed Oct 18, 2024

View reviewed changes

VincentRouvreau added 7 commits October 23, 2024 09:15

comment review: redundant comment line

2d44573

code review: const/non const version of simplex_data

99b37a3

code review: Remove _cofaces_simplex_range that was useless

2143026

code review: begin1 and end1 are now Const_dictionary_it

0f8a7b8

code review: DRY for nodes_by_label

f8de11a

Also test read only for_each_simplex when const

da24a13

[skip ci] Add some modification explanation as comment

94ca0d9

mglisse reviewed Oct 26, 2024

View reviewed changes

VincentRouvreau added 3 commits November 5, 2024 17:04

code review: std::as_const(*this) to avoid redirection

4277a64

code review: simplex_data should return a const ref

cbbdc8a

code review: clear_filtration is now const. Mark methods that are not…

3d21b0c

… thread-safe

hschreiber reviewed Nov 7, 2024

View reviewed changes

VincentRouvreau added 4 commits November 12, 2024 10:30

code review: add some tests for contiguous_vertices, simplex and comp…

38a751f

…lex_vertex_range

code review: prefer the use of assign_filtration(sh, filt)

0d500bc

code review: _to_node_it, simplex_data and self_siblings are no more …

85efd97

…static with a const/non const version if necessary

code review: add const test for has_children and root()

323e119

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const simplex tree unitary tests and fix operator== and != #1145

Const simplex tree unitary tests and fix operator== and != #1145

VincentRouvreau commented Oct 18, 2024

hschreiber commented Oct 18, 2024

VincentRouvreau commented Oct 18, 2024

hschreiber commented Oct 18, 2024

mglisse left a comment

mglisse Oct 18, 2024

VincentRouvreau Oct 23, 2024 •

edited

Loading

mglisse Oct 18, 2024 •

edited

Loading

VincentRouvreau Nov 12, 2024

mglisse Oct 18, 2024

VincentRouvreau Nov 12, 2024 •

edited

Loading

mglisse Oct 18, 2024

VincentRouvreau Oct 23, 2024 •

edited

Loading

mglisse Oct 26, 2024

VincentRouvreau Nov 5, 2024 •

edited

Loading

mglisse Oct 26, 2024

VincentRouvreau Nov 5, 2024 •

edited

Loading

mglisse Oct 26, 2024

mglisse Oct 26, 2024

VincentRouvreau Nov 12, 2024

mglisse Oct 26, 2024

VincentRouvreau Nov 5, 2024 •

edited

Loading

mglisse Oct 26, 2024

VincentRouvreau Nov 5, 2024 •

edited

Loading

hschreiber Nov 7, 2024

VincentRouvreau Nov 12, 2024 •

edited

Loading

hschreiber Nov 7, 2024

VincentRouvreau Nov 12, 2024 •

edited

Loading

hschreiber Nov 7, 2024

VincentRouvreau Nov 12, 2024

hschreiber Nov 7, 2024

VincentRouvreau Nov 12, 2024

		@@ -715,7 +719,7 @@ class Simplex_tree {
		static Simplex_data& simplex_data(Simplex_handle sh) {

	const Simplex_data simplex_data(Simplex_handle sh) const {
	const Simplex_data& simplex_data(Simplex_handle sh) const {

		// all mutable as their content has no impact on the content of the simplex tree itself
		// they correspond to some kind of cache or helper attributes.

Const simplex tree unitary tests and fix operator== and != #1145

Are you sure you want to change the base?

Const simplex tree unitary tests and fix operator== and != #1145

Conversation

VincentRouvreau commented Oct 18, 2024

hschreiber commented Oct 18, 2024

VincentRouvreau commented Oct 18, 2024

hschreiber commented Oct 18, 2024

mglisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

mglisse Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 5, 2024 • 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

Choose a reason for hiding this comment

VincentRouvreau Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentRouvreau Nov 12, 2024 • 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

Choose a reason for hiding this comment

VincentRouvreau Oct 23, 2024 •

edited

Loading

mglisse Oct 18, 2024 •

edited

Loading

VincentRouvreau Nov 12, 2024 •

edited

Loading

VincentRouvreau Oct 23, 2024 •

edited

Loading

VincentRouvreau Nov 5, 2024 •

edited

Loading

VincentRouvreau Nov 5, 2024 •

edited

Loading

VincentRouvreau Nov 5, 2024 •

edited

Loading

VincentRouvreau Nov 5, 2024 •

edited

Loading

VincentRouvreau Nov 12, 2024 •

edited

Loading

VincentRouvreau Nov 12, 2024 •

edited

Loading