Zigzag persistence part1 #917
Conversation
…l into persistence_matrix
…l into persistence_matrix
…n matrices are missing, the ones available are for container indexation. As the id interface is not usefull yet, to do later.
…eir characteristic
| /** | ||
| * @brief List of options used for the matrix maintained for the zigzag persistence computation. | ||
| */ | ||
| struct ZigzagPersistenceOptions { |
There was a problem hiding this comment.
What is the relation between this concept and PersistenceMatrixOptions?
| * \details The type ZigzagComplex::Simplex_key counts the number of | ||
| * insertions and deletions of simplices, which may be large in zigzag persistence and require | ||
| * more than 32 bits of storage. The type used (int, long, etc) should be chosen in | ||
| * consequence. Simplex_key must be signed. |
There was a problem hiding this comment.
Isn't that already said in the concept?
|
|
||
| protected: // note that we don't assume b_ <= d_ | ||
| int dim_; // homological dimension | ||
| value_type b_; // filtration value associated to birth index |
There was a problem hiding this comment.
This doesn't match with the comment above for birth() which says that b_ is the birth index.
| * closed for finite indices b and d, and open for left-infinite and/or | ||
| * right-infinite endpoints. | ||
| */ | ||
| struct filtration_value_interval : interval<Filtration_value> |
There was a problem hiding this comment.
Should we capitalize type names?
| if (Base::b_ == Base::d_) { | ||
| return 0; | ||
| } // otherwise inf - inf would return nan. | ||
| return std::abs(Base::b_ - Base::d_); |
There was a problem hiding this comment.
Is it possible to have d_ smaller than b_?
| } | ||
|
|
||
| /** | ||
| * @brief Returns the current persistence diagram ordered first by length, than by dimension, |
There was a problem hiding this comment.
Is it really the role of this function to sort the intervals? The cost is probably negligible compared to the rest, but if I don't care about the order or if I want a different order, I am not very happy paying for this useless sort. Actually, it looks like it calls _get_persistence_diagram which also sorts with a different order, so we always pay for 2 sorts?
| _retrieve_infinit_bars(diag); | ||
| } | ||
|
|
||
| std::stable_sort(diag.begin(), diag.end(), comp); |
There was a problem hiding this comment.
Can you add a comment explaining why stable_sort and not plain sort here?
| using Simplex_key = typename Complex::Simplex_key; /**< Key type, must be signed. */ | ||
| using Simplex_handle = typename Complex::Simplex_handle; /**< Simplex ID type in the complex. */ | ||
| using Vertex_handle = typename Complex::Vertex_handle; /**< Vertex ID type in the complex. */ | ||
| using Filtration_value = typename Complex::Filtration_value; /**< Filtration value type. */ |
There was a problem hiding this comment.
How are filtration values interpreted exactly? Operations have an index (operation 0 is inserting this vertex, operation 1 is inserting this other vertex, etc). Is the filtration value just a map from indexes to reals (or whatever type) that we bundle with this structure for convenience, without any meaning? Or does it have an actual semantic (for instance say 2 simplices removed at the same filtration value may be removed in any order)?
| matrix_type matrix_; /**< Matrix storing a base of the current chain complex. */ | ||
| std::unordered_map<index, int> births_; /**< Map simplex index in F to corresponding birth. */ | ||
| birth_ordering birth_ordering_; /**< Maintains <b ordering of the births. */ | ||
| std::list<index_interval> persistence_diagram_; /**< Stores current closed persistence intervals. */ |
There was a problem hiding this comment.
Do you specifically need std::list, or would std::vector work?
(https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon2-prefer-using-stl-vector-by-default-unless-you-have-a-reason-to-use-a-different-container)
| Complex cpx_; /**< Complex in which the current simplices are stored. */ | ||
| int dim_max_; /**< Maximal dimension of a bar to record. */ | ||
| matrix_type matrix_; /**< Matrix storing a base of the current chain complex. */ | ||
| std::unordered_map<index, int> births_; /**< Map simplex index in F to corresponding birth. */ |
There was a problem hiding this comment.
Plain int, there is no relevant typedef?
New module to compute zigzag persistence. To simplify the review process, this version contains only the main functionality, that is to compute the barcode.
Following functionalities will be added in separated PRs:
The code is based on PR #401 and requires PR #886 and draft #669.