Zigzag persistence part2 #953
Conversation
… oscillating_rips
|
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. |
| @@ -294,6 +294,29 @@ | |||
| </tr> | |||
| </table> | |||
|
|
|||
| ### Oscillating Rips Filtrations | |||
There was a problem hiding this comment.
Do we want this directly on the main page, or in the Rips page (which already has sparse Rips for instance)?
There was a problem hiding this comment.
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...
| * 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -52,6 +52,43 @@ namespace zigzag_persistence { | |||
| * which can be processed without overreaching memory space. For this purpose, it is possible to feed the module with | |||
| * 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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>.
| * 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It was just to contrast it with the vector version, i.e., to highlight that iterator_range 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.
| * gudhi/Zigzag_persistence/oscillating_rips_edge_ranges.h | ||
| * @brief Custom iterator over the edges of an oscillating rips filtration. | ||
| * | ||
| * It inherits from boost::iterator_facade. |
There was a problem hiding this comment.
User doesn't care?
They may care what the iterator_category is though.
There was a problem hiding this comment.
I give the iterator category in the description of the begin() method. But it could indeed be also a good addition here. Even though, I don't think that the description of this class interest the user at all (only constructors are public), as it is mostly handled by boost::iterator_facade. Therefore the mention.
| * 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 |
There was a problem hiding this comment.
Isn't range_ redundant in the name?
| * - @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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It is explained in the description of the classes themselves. But I am okay with repeating it here.
Co-authored-by: Marc Glisse <[email protected]>
Co-authored-by: Marc Glisse <[email protected]>
Co-authored-by: Marc Glisse <[email protected]>
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...