Zigzag persistence part2 by hschreiber · Pull Request #953 · GUDHI/gudhi-devel

hschreiber · 2023-09-01T07:42:52Z

Follow up of PR #917 with the addition of the oscillating Rips iterator.

The PR is still a draft as unit tests are still missing (and the doc main page could be better). I want to ask a few things @ClementMaria when he comes back before doing them.
As this depends of #917 which itself depends on #669 there is still enough to do before then...

… oscillating_rips

…illating_rips

…ting_rips

hschreiber · 2025-01-14T17:41:16Z

I still have to re-read trough the documentation to be sure that nothing was forgotten with the last changes, but otherwise the module should be ready to be reviewed. The unit tests were finally added and I completed the introduction of the oscillating rips filtrations.

mglisse

Some early comments, or reminders to discuss some things, but I haven't really read the PR yet.

mglisse · 2025-02-09T13:28:16Z

  </tr>
 </table>

+### Oscillating Rips Filtrations


Do we want this directly on the main page, or in the Rips page (which already has sparse Rips for instance)?

I don't have a strong opinion about it. But if not every variant is displayed on the main page, we should perhaps add a mention to them in the module description of the main page. Something like:

The (Vietoris-)Rips complex is a filtered simplicial complex where every tuple of points appears with its diameter as filtration value. Alternatively, it is defined by the cliques of a graph, usually representing a metric. This complex can be built from a point cloud and a distance function, or from a distance matrix. Implemented variants: - Vietoris-Rips complex - Sparse Rips complex - Oscillating Rips filtration

They kind of go under otherwise...

mglisse · 2025-02-09T13:35:55Z

+ * If \f$ P \f$ is the set of points generating the rips filtration, \f$ P_i \f$ corresponds to the subset containing
+ * the \f$ i \f$ first points. So, at each forward inclusion, one vertex is added.
+ *
+ * The superscript of \f$ \mathcal{R} \f$ corresponds to the filtration values which will be associated to the cycles.


Maybe say that later? First we describe the zigzag. And once that's done, we can say that we (arbitrarily?) use this as "filtration value" for the output barcode.
I assume you chose epsilon because it is monotonous, unlike the max edge length parameter, while still being homogeneous to a distance, unlike the index?

I was not the one who chose the epsilon values as filtration values, but yes, I think the reason was going in this direction. And it does make sense, so I didn't contested it.

I presented it like that, because I didn't wanted to confuse the reader with theoretical details, but describing directly how it was actually implemented in Gudhi (while still making clear how the filtration looks like). In particular, because there exists other implemented variants which don't chose the exact same parametrization (for example, Dionysus, even though I can't find its documentation anymore...).

But I could invert the positions of this paragraph with the one below and be more explicit about the fact that the choice of filtration is not part of the definition, just our choice.

mglisse · 2025-02-09T13:42:43Z

 * information about the filtration "on the fly" to avoid loading the whole filtration at once. Information about the
 * current barcode can be retrieved between any steps via callback methods.
+ *
+ * \subsection zigzagrips Oscillating Rips


Do we want it on this page, which is more about persistence, while it is more about the geometry of building a zigzag filtration?
I am asking because Rips_complex and Persistent_cohomology are in 2 clearly separated pages.

Good question. I always saw it as an extension of the zigzag module, so I found it natural to put it there. But when you think about it without this point of view, the placement is a bit weird indeed. We could put it together with the Rips complex like you suggested above.

But then, should I also move the actual code ? For now the import of the ranges is via #include <gudhi/Zigzag_persistence/oscillating_rips_simplex_ranges.h> and only the helper method is imported directly with #include <gudhi/Oscillating_rips_persistence.h>.

mglisse · 2025-02-09T14:13:42Z

+ * Both multipliers have to be specified by the user.
+ *
+ * The construction is based on two types of classes:
+ * - @ref Oscillating_rips_edge_iterator_range and @ref Oscillating_rips_edge_vector_range_constructor computes the


Why not shorten Oscillating_rips_edge_iterator_range to Oscillating_rips_edge_range? IIUC there is a range, begin on it returns Oscillating_rips_edge_iterator, an iterator is the thing that iterates on the elements of a range, everything works?

It was just to contrast it with the vector version, i.e., to highlight that iterator_range $\neq$ vector_range. Same idea goes for your question below "Isn't range_ redundant in the name?".
But I am not super happy about my choice of names. So, any suggestion is welcome. It would just be nice if the difference between the two classes remain clear from the name. If possible.

mglisse · 2025-02-09T14:33:05Z

+ * use @ref Identity_edge_modifier. Default value: @ref Identity_edge_modifier.
+ */
+template <typename Filtration_value, class EdgeModifier = Identity_edge_modifier>
+class Oscillating_rips_edge_vector_range_constructor


Isn't range_ redundant in the name?

mglisse · 2025-02-09T14:35:34Z

+ * - @ref Oscillating_rips_edge_iterator_range and @ref Oscillating_rips_edge_vector_range_constructor computes the
+ * range of inserted and removed vertices and edges in the filtration based on the elements descipted above.


Is it explained somewhere that one stores the whole filtration in a vector while the other computes the edges lazily as you iterate on it?

It is explained in the description of the classes themselves. But I am okay with repeating it here.

Co-authored-by: Marc Glisse <marc.glisse@inria.fr>

mglisse

Random comments. Some may be irrelevant because I haven't read the full PR yet, but since I am going to disappear for a couple days again, I might as well post them.

mglisse · 2025-04-14T11:08:31Z

+#ifndef CONCEPT_ZZ_POINT_RANGE_H_
+#define CONCEPT_ZZ_POINT_RANGE_H_
+
+/** @file PointRange.h


Could you remind me what this is for? I think doxygen knows the name of the file it is processing, and the user shouldn't care about the name of the file where the concept is documented.

It helps Doxygen to index the files better and avoids a few @ref problems. It is one of those Doxygen properties, where you don't really need it most of the time, but at some seemly random point, it will refuse referencing a method because it was missing.
It also adds a description in the file list on the website.

mglisse · 2025-04-14T11:17:42Z

+   * @param f Value to modify.
+   * @return The modified value of @p f.
+   */
+  static Filtration_value apply_modifier(Filtration_value f);


static means we won't be able to use an EdgeModifier that stores data (say PowerEdgeModifier that has a field storing the desired exponent at runtime). If we don't expect to use anything other than identity or sqrt, I guess that's ok.

True. But the way it is implemented for now, the user never passes an instance of Edge_modifier to a method, only the type, from which we call Edge_modifier::apply_inverse_modifier etc. We would need to change that.
If it becomes useful, we can still add it then, I think. With an optional argument for example which won't break retro-compatibility.

mglisse · 2025-04-14T13:29:12Z

+
+ private:
+  const std::vector<Filtration_value>* epsilonValues_;                                 /**< Epsilon values. */
+  const std::vector<std::vector<std::pair<int, Filtration_value> > >* distanceMatrix_; /**< Distance matrix. */


IIUC this is a lower triangular matrix without diagonal (m[i][j] only valid if j<i), maybe add that as a comment?

If we wanted to, we could make it a flat vector, element (i,j) would have index i*(i-1)/2+j or a similar formula, but it may not be interesting, and it is private so can be changed later anyway.

I didn't wanted to modify too much the initial implementation, but making the matrix flat could be a good idea. But as you say, it is private anyway as Oscillating_rips_edge_iterator_base is not really supposed to be used externally, so we can look at it later. I kept the documentation of this class public mainly to keep references to it possible, but perhaps I should just remove them?

mglisse · 2025-04-14T13:36:07Z

+  VECTOR,     /**< The edges are computed all at once and stored in a vector. */
+  BOOST_RANGE /**< The edges are computed one by one at each increment of the
+                   range iterator and therefore not stored. */


I don't feel that BOOST_RANGE really conveys what this option does (I could easily wrap a vector in a boost range).
eager vs lazy?
precomputed/stored vs generated?
I don't have a very good suggestion...

Changed it to ALL_AT_ONCE and ONE_BY_ONE. A bit ugly, but better than before, I guess? What do you think?

hschreiber and others added 22 commits July 28, 2023 18:45

rips edge iterator

4f06e5c

Merge remote-tracking branch 'origin/simplex_tree_zigzag_option' into…

9eb1e01

… oscillating_rips

Merge remote-tracking branch 'origin/stable_simplex_handles' into osc…

0dcf241

…illating_rips

oscillating rips range

5e530f7

Merge remote-tracking branch 'origin/persistence_matrix' into oscilla…

40bb36a

…ting_rips

oscillating rips persistence, example and benchmark

9b0a7cc

doc

63aad6a

doc

951cbb8

merge updated upstream

d63e697

Merge branch 'GUDHI:master' into oscillating_rips

b640b89

Merge branch 'GUDHI:master' into oscillating_rips

52376df

doc

8f2dc9c

cleanup

67637b9

merge upstream

ba7ee4d

merge upstream

f0978fc

name fix

d8a8273

add std:: to size_t

da86ef5

merge upstream part1

0655678

merge upstream part2

3618262

oscillating rips unitary tests part 1

656435e

oscillating rips unitary tests part 2

8e1512a

doc

f76a682

hschreiber marked this pull request as ready for review January 14, 2025 17:39

hschreiber added 4 commits January 14, 2025 18:44

biblio misplaced

9a526f6

doc + removal of unnecessary flag in edge modifier

aac903d

oscillating rips: more intuitive organization + light iterators

b69b288

doc

2e973f1

mglisse reviewed Feb 9, 2025

View reviewed changes

Merge branch 'GUDHI:master' into oscillating_rips

f09a1a0

hschreiber and others added 5 commits February 13, 2025 13:45

Update src/Zigzag_persistence/doc/Intro_zigzag_persistence.h

6d5d901

Co-authored-by: Marc Glisse <marc.glisse@inria.fr>

Update src/Zigzag_persistence/doc/Intro_zigzag_persistence.h

3cf9a49

Co-authored-by: Marc Glisse <marc.glisse@inria.fr>

Update src/Zigzag_persistence/doc/Intro_zigzag_persistence.h

ba0d7db

Co-authored-by: Marc Glisse <marc.glisse@inria.fr>

doc corrections

b0ebdcb

upstream merge

9ae8eed

mglisse reviewed Apr 14, 2025

View reviewed changes

hschreiber and others added 5 commits September 15, 2025 17:35

Merge branch 'GUDHI:master' into oscillating_rips

9485c11

few name changes + doc

eb83b19

Merge branch 'GUDHI:master' into oscillating_rips

ab265aa

Merge branch 'GUDHI:master' into oscillating_rips

a23a37b

Merge branch 'GUDHI:master' into oscillating_rips

901a785

		* - @ref Oscillating_rips_edge_iterator_range and @ref Oscillating_rips_edge_vector_range_constructor computes the
		* range of inserted and removed vertices and edges in the filtration based on the elements descipted above.

Uh oh!

Conversation

hschreiber commented Sep 1, 2023

Uh oh!

hschreiber commented Jan 14, 2025

Uh oh!

mglisse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mglisse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants