Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Persistence Matrix] replacing static_cast(-1) by method #1153

Merged
merged 4 commits into from
Feb 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/Persistence_matrix/concept/PersistenceMatrixColumn.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,19 @@ class PersistenceMatrixColumn :

/**
* @brief Reorders the column with the given map of row indices. Also changes the column index stored in the
* entries if row access is enabled and @p columnIndex is not -1.
* entries if row access is enabled and @p columnIndex is not the @ref Matrix::get_null_value "null index".
*
* Only useful for @ref basematrix "base" and @ref boundarymatrix "boundary matrices" using lazy swaps.
*
* @tparam Row_index_map Map with an %at() method.
* @param valueMap Map such that `valueMap.at(i)` indicates the new row index of the entry
* at current row index `i`.
* @param columnIndex New @ref MatIdx column index of the column. If -1, the index does not change. Ignored if
* the row access is not enabled. Default value: -1.
* @param columnIndex New @ref MatIdx column index of the column. If @ref Matrix::get_null_value "null index",
* the index does not change. Ignored if the row access is not enabled.
* Default value: @ref Matrix::get_null_value "null index".
*/
template <class Row_index_map>
void reorder(const Row_index_map& valueMap, [[maybe_unused]] Index columnIndex = -1);
void reorder(const Row_index_map& valueMap, [[maybe_unused]] Index columnIndex = Matrix::get_null_value<Index>());
/**
* @brief Zeros/empties the column.
*
Expand All @@ -275,11 +276,12 @@ class PersistenceMatrixColumn :
void clear(ID_index rowIndex);

/**
* @brief Returns the row index of the pivot. If the column does not have a pivot, returns -1.
* @brief Returns the row index of the pivot. If the column does not have a pivot,
* returns @ref Matrix::get_null_value "null index".
*
* Only useful for @ref boundarymatrix "boundary" and @ref chainmatrix "chain matrices".
*
* @return Row index of the pivot or -1.
* @return Row index of the pivot or @ref Matrix::get_null_value "null index".
*/
ID_index get_pivot();
/**
Expand Down Expand Up @@ -429,7 +431,7 @@ class PersistenceMatrixColumn :
/**
* @brief Adds a copy of the given entry at the end of the column. It is therefore assumed that the row index
* of the entry is higher than the current pivot of the column. Not available for @ref chainmatrix "chain matrices"
* and is only needed for @ref rumatrix "RU matrices".
* and is only needed for @ref boundarymatrix "RU matrices".
*
* @param entry Entry to push back.
*/
Expand Down
31 changes: 20 additions & 11 deletions src/Persistence_matrix/include/gudhi/Matrix.h
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,15 @@ class Matrix {
*/
using Element = typename Field_operators::Element;
using Characteristic = typename Field_operators::Characteristic;


/**
* @brief Returns value from a type when not set.
*/
template <typename T>
static constexpr const T get_null_value() {
return -1;
}

/**
* @brief Type for a bar in the computed barcode. Stores the birth, death and dimension of the bar.
*/
Expand Down Expand Up @@ -367,7 +375,7 @@ class Matrix {
//purposely triggers operators() instead of operators(characteristic) as the "dummy" values for the different
//operators can be different from -1.
Column_zp_settings(Characteristic characteristic) : operators(), entryConstructor() {
if (characteristic != static_cast<Characteristic>(-1)) operators.set_characteristic(characteristic);
if (characteristic != get_null_value<Characteristic>()) operators.set_characteristic(characteristic);
}
Column_zp_settings(const Column_zp_settings& toCopy)
: operators(toCopy.operators.get_characteristic()), entryConstructor() {}
Expand Down Expand Up @@ -592,7 +600,7 @@ class Matrix {
* @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, Characteristic characteristic = static_cast<Characteristic>(-1));
Matrix(unsigned int numberOfColumns, Characteristic characteristic = Matrix::get_null_value<Characteristic>());
/**
* @brief Constructs a new empty matrix with the given comparator functions. Only available when those comparators
* are necessary.
Expand Down Expand Up @@ -671,7 +679,7 @@ class Matrix {
Matrix(unsigned int numberOfColumns,
const std::function<bool(Pos_index,Pos_index)>& birthComparator,
const std::function<bool(Pos_index,Pos_index)>& deathComparator,
Characteristic characteristic = static_cast<Characteristic>(-1));
Characteristic characteristic = Matrix::get_null_value<Characteristic>());
/**
* @brief Copy constructor.
*
Expand Down Expand Up @@ -765,7 +773,7 @@ class Matrix {
* chains used to reduce the boundary. Otherwise, nothing.
*/
template <class Boundary_range = Boundary>
Insertion_return insert_boundary(const Boundary_range& boundary, Dimension dim = -1);
Insertion_return insert_boundary(const Boundary_range& boundary, Dimension dim = Matrix::get_null_value<Dimension>());
/**
* @brief Only available for @ref mp_matrices "non-basic matrices".
* It does the same as the other version, but allows the boundary cells to be identified without restrictions
Expand All @@ -787,7 +795,8 @@ class Matrix {
* chains used to reduce the boundary. Otherwise, nothing.
*/
template <class Boundary_range = Boundary>
Insertion_return insert_boundary(ID_index cellIndex, const Boundary_range& boundary, Dimension dim = -1);
Insertion_return insert_boundary(ID_index cellIndex, const Boundary_range& boundary,
Dimension dim = Matrix::get_null_value<Dimension>());

/**
* @brief Returns the column at the given @ref MatIdx index.
Expand Down Expand Up @@ -1510,7 +1519,7 @@ template <class PersistenceMatrixOptions>
inline void Matrix<PersistenceMatrixOptions>::set_characteristic(Characteristic characteristic)
{
if constexpr (!PersistenceMatrixOptions::is_z2) {
if (colSettings_->operators.get_characteristic() != static_cast<Characteristic>(-1)) {
if (colSettings_->operators.get_characteristic() != get_null_value<Characteristic>()) {
std::cerr << "Warning: Characteristic already initialised. Changing it could lead to incoherences in the matrix "
"as the modulo was already applied to values in existing columns.";
}
Expand All @@ -1524,7 +1533,7 @@ template <class Container>
inline void Matrix<PersistenceMatrixOptions>::insert_column(const Container& column)
{
if constexpr (!PersistenceMatrixOptions::is_z2){
GUDHI_CHECK(colSettings_->operators.get_characteristic() != static_cast<Characteristic>(-1),
GUDHI_CHECK(colSettings_->operators.get_characteristic() != get_null_value<Characteristic>(),
std::logic_error("Matrix::insert_column - Columns cannot be initialized if the coefficient field "
"characteristic is not specified."));
}
Expand All @@ -1540,7 +1549,7 @@ template <class Container>
inline void Matrix<PersistenceMatrixOptions>::insert_column(const Container& column, Index columnIndex)
{
if constexpr (!PersistenceMatrixOptions::is_z2){
GUDHI_CHECK(colSettings_->operators.get_characteristic() != static_cast<Characteristic>(-1),
GUDHI_CHECK(colSettings_->operators.get_characteristic() != get_null_value<Characteristic>(),
std::logic_error("Matrix::insert_column - Columns cannot be initialized if the coefficient field "
"characteristic is not specified."));
}
Expand All @@ -1558,7 +1567,7 @@ inline typename Matrix<PersistenceMatrixOptions>::Insertion_return
Matrix<PersistenceMatrixOptions>::insert_boundary(const Boundary_range& boundary, Dimension dim)
{
if constexpr (!PersistenceMatrixOptions::is_z2){
GUDHI_CHECK(colSettings_->operators.get_characteristic() != static_cast<Characteristic>(-1),
GUDHI_CHECK(colSettings_->operators.get_characteristic() != get_null_value<Characteristic>(),
std::logic_error("Matrix::insert_boundary - Columns cannot be initialized if the coefficient field "
"characteristic is not specified."));
}
Expand All @@ -1578,7 +1587,7 @@ Matrix<PersistenceMatrixOptions>::insert_boundary(ID_index cellIndex,
Dimension dim)
{
if constexpr (!PersistenceMatrixOptions::is_z2){
GUDHI_CHECK(colSettings_->operators.get_characteristic() != static_cast<Characteristic>(-1),
GUDHI_CHECK(colSettings_->operators.get_characteristic() != get_null_value<Characteristic>(),
std::logic_error("Matrix::insert_boundary - Columns cannot be initialized if the coefficient field "
"characteristic is not specified."));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ class Base_matrix : public Master_matrix::template Base_swap_option<Base_matrix<
* @param dim Ignored.
*/
template <class Boundary_range>
void insert_boundary(const Boundary_range& boundary, Dimension dim = -1);
void insert_boundary(const Boundary_range& boundary,
Dimension dim = Master_matrix::template get_null_value<Dimension>());
/**
* @brief Returns the column at the given @ref MatIdx index.
* The type of the column depends on the choosen options, see @ref PersistenceMatrixOptions::column_type.
Expand Down Expand Up @@ -438,7 +439,7 @@ template <class Master_matrix>
template <class Boundary_range>
inline void Base_matrix<Master_matrix>::insert_boundary(const Boundary_range& boundary, Dimension dim)
{
if (dim == -1) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;
if (dim == Master_matrix::template get_null_value<Dimension>()) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;
//TODO: dim not actually stored right now, so either get rid of it or store it again
_insert(boundary, nextInsertIndex_++, dim);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ class Base_matrix_with_column_compression : protected Master_matrix::Matrix_row_
* @param dim Ignored.
*/
template <class Boundary_range>
void insert_boundary(const Boundary_range& boundary, Dimension dim = -1);
void insert_boundary(const Boundary_range& boundary,
Dimension dim = Master_matrix::template get_null_value<Dimension>());
/**
* @brief Returns the column at the given @ref MatIdx index.
* The type of the column depends on the choosen options, see @ref PersistenceMatrixOptions::column_type.
Expand Down Expand Up @@ -450,7 +451,7 @@ inline void Base_matrix_with_column_compression<Master_matrix>::insert_boundary(
// handles a dimension which is not actually stored.
// TODO: verify if this is not a problem for the cohomology algorithm and if yes,
// change how Column_dimension_option is choosen.
if (dim == -1) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;
if (dim == Master_matrix::template get_null_value<Dimension>()) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;

if constexpr (Master_matrix::Option_list::has_row_access && !Master_matrix::Option_list::has_removable_rows) {
if (boundary.begin() != boundary.end()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ class Boundary_matrix : public Master_matrix::Matrix_dimension_option,
* @return The @ref MatIdx index of the inserted boundary.
*/
template <class Boundary_range = Boundary>
Index insert_boundary(const Boundary_range& boundary, Dimension dim = -1);
Index insert_boundary(const Boundary_range& boundary,
Dimension dim = Master_matrix::template get_null_value<Dimension>());
/**
* @brief It does the same as the other version, but allows the boundary cells to be identified without restrictions
* except that all IDs have to be strictly increasing in the order of filtration. Note that you should avoid then
Expand All @@ -161,7 +162,8 @@ class Boundary_matrix : public Master_matrix::Matrix_dimension_option,
* @return The @ref MatIdx index of the inserted boundary.
*/
template <class Boundary_range = Boundary>
Index insert_boundary(ID_index cellIndex, const Boundary_range& boundary, Dimension dim = -1);
Index insert_boundary(ID_index cellIndex, const Boundary_range& boundary,
Dimension dim = Master_matrix::template get_null_value<Dimension>());
/**
* @brief Returns the column at the given @ref MatIdx index.
* The type of the column depends on the choosen options, see @ref PersistenceMatrixOptions::column_type.
Expand Down Expand Up @@ -384,7 +386,7 @@ class Boundary_matrix : public Master_matrix::Matrix_dimension_option,

template <class Master_matrix>
inline Boundary_matrix<Master_matrix>::Boundary_matrix(Column_settings* colSettings)
: Dim_opt(-1),
: Dim_opt(Master_matrix::template get_null_value<Dimension>()),
Swap_opt(),
Pair_opt(),
RA_opt(),
Expand All @@ -396,7 +398,7 @@ template <class Master_matrix>
template <class Boundary_range>
inline Boundary_matrix<Master_matrix>::Boundary_matrix(const std::vector<Boundary_range>& orderedBoundaries,
Column_settings* colSettings)
: Dim_opt(-1),
: Dim_opt(Master_matrix::template get_null_value<Dimension>()),
Swap_opt(orderedBoundaries.size()),
Pair_opt(),
RA_opt(orderedBoundaries.size()),
Expand All @@ -413,7 +415,7 @@ inline Boundary_matrix<Master_matrix>::Boundary_matrix(const std::vector<Boundar
template <class Master_matrix>
inline Boundary_matrix<Master_matrix>::Boundary_matrix(unsigned int numberOfColumns,
Column_settings* colSettings)
: Dim_opt(-1),
: Dim_opt(Master_matrix::template get_null_value<Dimension>()),
Swap_opt(numberOfColumns),
Pair_opt(),
RA_opt(numberOfColumns),
Expand Down Expand Up @@ -471,7 +473,7 @@ template <class Boundary_range>
inline typename Boundary_matrix<Master_matrix>::Index Boundary_matrix<Master_matrix>::insert_boundary(
ID_index cellIndex, const Boundary_range& boundary, Dimension dim)
{
if (dim == -1) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;
if (dim == Master_matrix::template get_null_value<Dimension>()) dim = boundary.size() == 0 ? 0 : boundary.size() - 1;

_orderRowsIfNecessary();

Expand Down Expand Up @@ -543,7 +545,7 @@ inline typename Boundary_matrix<Master_matrix>::Index Boundary_matrix<Master_mat
static_assert(Master_matrix::Option_list::has_removable_columns,
"'remove_last' is not implemented for the chosen options.");

if (nextInsertIndex_ == 0) return -1; // empty matrix
if (nextInsertIndex_ == 0) return Master_matrix::template get_null_value<Index>(); // empty matrix
--nextInsertIndex_;

//updates dimension max
Expand All @@ -558,7 +560,7 @@ inline typename Boundary_matrix<Master_matrix>::Index Boundary_matrix<Master_mat
pivot = it->second.get_pivot();
if constexpr (activeSwapOption) {
// if the removed column is positive, the pivot won't change value
if (Swap_opt::rowSwapped_ && pivot != static_cast<ID_index>(-1)) {
if (Swap_opt::rowSwapped_ && pivot != Master_matrix::template get_null_value<ID_index>()) {
Swap_opt::_orderRows();
pivot = it->second.get_pivot();
}
Expand All @@ -568,7 +570,7 @@ inline typename Boundary_matrix<Master_matrix>::Index Boundary_matrix<Master_mat
pivot = matrix_[nextInsertIndex_].get_pivot();
if constexpr (activeSwapOption) {
// if the removed column is positive, the pivot won't change value
if (Swap_opt::rowSwapped_ && pivot != static_cast<ID_index>(-1)) {
if (Swap_opt::rowSwapped_ && pivot != Master_matrix::template get_null_value<ID_index>()) {
Swap_opt::_orderRows();
pivot = matrix_[nextInsertIndex_].get_pivot();
}
Expand Down
Loading
Loading