Persistence matrix module #669
Conversation
| } | ||
|
|
||
| template<unsigned int characteristic> | ||
| inline void Zp_field_element<characteristic>::_multiply(unsigned int v) |
There was a problem hiding this comment.
That doesn't look very efficient. Good thing it is a negligible part of the computation anyway...
There was a problem hiding this comment.
It is the most efficient version I could find that computes the modulo multiplication without any overflow. But I agree that this class will probably never be used with such high numbers that an overflow is possible, so it is kind of useless. I will test at some point if this creates a real overhead or not.
src/Persistence_matrix/include/gudhi/boundary_matrix/base_matrix_0000.h
Outdated
Show resolved
Hide resolved
src/Persistence_matrix/include/gudhi/boundary_matrix/base_matrix_0000.h
Outdated
Show resolved
Hide resolved
src/Persistence_matrix/include/gudhi/boundary_matrix/base_matrix_0000.h
Outdated
Show resolved
Hide resolved
…l into persistence_matrix
…l into persistence_matrix
src/Persistence_matrix/include/gudhi/Persistence_matrix/columns/heap_column.h
Outdated
Show resolved
Hide resolved
…tion of operations in Multifields
| * @warning As the member is static, they can eventually be problems if the matrix is duplicated in several threads. | ||
| * If this become necessary, the static should be removed in the future. | ||
| */ | ||
| inline static Simple_object_pool<Column_type> columnPool_; |
There was a problem hiding this comment.
It seems like a big problem if we cannot even have 2 threads that both compute persistence on completely independent filtrations.
There was a problem hiding this comment.
Ah, I forgot about that. I think, we can just remove the static from it and it should work just fine as it is only used inside the compressed matrix.
There was a problem hiding this comment.
IIRC you have a comment about that elsewhere in the file (near swap maybe?) saying that it may be a problem.
There was a problem hiding this comment.
True, but it is not that much of a problem. It was just to remind me that if I remove the static, I should also swap the pools in swap as the pointers won't belong to the right pool otherwise.
There was a problem hiding this comment.
So the right thing will happen if
- I swap 2 non-empty matrices
- I swap columns that don't belong to the same matrix
? Good then.
There was a problem hiding this comment.
If two non empty matrices are swapped, you just have to be careful that the matrices own the right pool at the end. If two columns from different matrices are swapped, their content are swapped but not their addresses, so it is also fine. Were problem can arise when swapping two columns from different matrices, is at the row access, as the cells in the rows will not be swapped (you should first unlink a column and relink it after swap). But this has nothing to do with this pool. Nevertheless, I realized that I never mentioned this in the doc. I could also just add the linking/unlinking in the swap operator.
| * @ref set_characteristic before calling for the first time a method needing it. | ||
| * Ignored if @ref PersistenceMatrixOptions::is_z2 is true. | ||
| */ | ||
| Matrix(unsigned int numberOfColumns, |
There was a problem hiding this comment.
Should all the unsigned int used for columns be index_type or some related typedef?
There was a problem hiding this comment.
I wanted to use a native type here to indicate that numberOfColumns is not something stored (and therefore doesn't really need to be optimized in size). And the chances are not very high that someone wants to use more than max unsigned int columns. Otherwise, index would be the right type to use.
I am more bothered by the fact that I use unsigned int in one constructor and int in another. This makes no sense.
There was a problem hiding this comment.
I wanted to use a native type here to indicate that
numberOfColumnsis not something stored (and therefore doesn't really need to be optimized in size).
As a user, that's not the message I understand when I read unsigned int, almost the opposite. std::size_t or std::intmax_t (or maybe ptrdiff_t) would be closer.
the chances are not very high that someone wants to use more than max unsigned int columns.
"640K [of RAM] ought to be enough for anybody" 😉
A few years after starting Gudhi, we received a bug report because the code failed on a simplicial complex with 3 billion simplices. Sure, that required an awful lot of memory to reproduce, but I don't see a good reason to hardcode a limit of 4 billions (the default options can still limit the computation to 4G, that's fine).
| if constexpr (isNonBasic && !PersistenceMatrixOptions::is_of_boundary_type && | ||
| PersistenceMatrixOptions::column_indexation_type == Column_indexation_types::CONTAINER) | ||
| return matrix_.insert_boundary(boundary, dim); | ||
| else | ||
| matrix_.insert_boundary(boundary, dim); |
There was a problem hiding this comment.
If matrix_.insert_boundary(boundary, dim); returns void exactly in the right cases, we could just write return matrix_.insert_boundary(boundary, dim); without need for 2 cases.
(if sometimes matrix_.insert_boundary(boundary, dim); returns a vector but we don't want to return that vector, then forget this comment)
| using tmp_column_type = typename std::conditional< | ||
| Master_matrix::Option_list::is_z2, | ||
| std::set<id_index>, | ||
| std::map<id_index, Field_element_type> | ||
| >::type; |
There was a problem hiding this comment.
It is surprising that this is not one of the types defined in Persistence_matrix/columns.
| * | ||
| * @brief List of column types. | ||
| */ | ||
| enum Column_types { |
There was a problem hiding this comment.
Is it possible for a user to use their own column type? There is a concept of what such a column type must look like, but from the use of this enum, it doesn't look like a user has any way of telling matrix to use it.
The matrix module is not finished yet, but I would like to have feedback on the structure, in particular on the way the different options are handled.
The options are listed in options.h and they are processed in matrix.h.
The basic functions of a matrix can also be found in matrix.h, and the more specialized functions are in their respective header (*_pairing, *_rep_cycles, *_vine_swap).
There are three different types of matrices:
In example/comp_test.cpp is a temporary example of how to call all different options implemented yet. The idea was to call all possible functions with trivial parameters to verify that everything compiles correctly, but not to test the functions.