Some cubical improvements

[VincentRouvreau]

From a c++ user point of view

1. It would be nice if a cubical (periodic or not) could be constructed from an initializer_list. Some thing like:

using namespace Gudhi::cubical_complex;
using Cubical = Bitmap_cubical_complex<Bitmap_cubical_complex_base<double>>

Cubical cub1({2, 2}, {1, 2, 3, 4});

using Periodic_cubical = Bitmap_cubical_complex<Bitmap_cubical_complex_periodic_boundary_conditions_base<double>>
Periodic_cubical pcub2({2, 2}, {1, 2, 3, 4}, {true, false});

2. Homogenize the behavior and messages for construction exceptions:

Cubical cub1({2, 2}, {1, 2, 3, 4, 5});
// Error in constructor Bitmap_cubical_complex_base ( std::vector<unsigned> sizes_in_following_directions, std::vector<T> top_dimensional_cells ). Number of top dimensional elements that follow from sizes_in_following_directions vector is different from the size of top_dimensional_cells vector.
// terminate called after throwing an instance of 'std::invalid_argument'
//   what():  Error in constructor Bitmap_cubical_complex_base( std::vector<unsigned> sizes_in_following_directions,std::vector<T> top_dimensional_cells ). Number of top dimensional elements that follow from sizes_in_following_directions vector is different from the size of top_dimensional_cells vector.
// Aborted (core dumped)

Cubical cub2({2, 2}, {1, 2, 3, 4, 5}, false);
// Error in constructor Bitmap_cubical_complex_base ( std::vector<unsigned> sizes_in_following_directions, std::vector<T> vertices ). Number of vertices elements that follow from sizes_in_following_directions vector is different from the size of vertices vector.
// terminate called after throwing an instance of 'std::invalid_argument'
//   what():  Error in constructor Bitmap_cubical_complex_base( std::vector<unsigned> sizes_in_following_directions,std::vector<T> vertices ). Number of vertices elements that follow from sizes_in_following_directions vector is different from the size of vertices vector.
// Aborted (core dumped)

Here, the code to detect the exception is duplicated. The code could be factorized and the message could be more generic.

Note that this behavior is not the same for the periodic code:

Periodic_cubical pcub1({2, 2}, {1, 2, 3, 4, 5}, {true, false}); // No exception, but it should because (2 x 2) != cells.size()
Periodic_cubical pcub2({2, 2}, {1, 2, 3, 4}, {true});  // No exception, but it should because dimensions.size() != periodic_directions.size()

If the behavior were similar it would help to fix #792

3. Constructors for periodic/non-periodic

using namespace Gudhi::cubical_complex;
using Cubical = Bitmap_cubical_complex<Bitmap_cubical_complex_base<double>>

Cubical cub1({2, 2}, {1, 2, 3, 4}, {true, false});  // construct a non-periodic cubical complex

using Periodic_cubical = Bitmap_cubical_complex<Bitmap_cubical_complex_periodic_boundary_conditions_base<double>>
Periodic_cubical pcub2({2, 2}, {1, 2, 3, 4});  // construct a periodic cubical complex with no periodic direction - A non-periodic would be faster

Here, we could make the compilation fails in these cases. With a level of indirection, we could make new interfaces for cubical that is using the correct class in function of the parameters given by the user.

From a Python user point of view

4. (cf. C++ 2.) The std::cerr message also appears in Python which is not what we want IMO.

5. Nanobind ndarray does not provide .begin(), nor .end() methods, it only provides .data() and .size() methods, which, that if we do not want to copy data structures, cubical should provide also constructor like:

  template<class DimensionIterator, class CellIterator>
  Bitmap_cubical_complex(const DimensionIterator dim_begin, const DimensionIterator dim_end,
                         const CellIterator cell_begin, const CellIterator cell_end,
                         bool input_top_cells = true)

[mglisse]

1. It would be nice if a cubical (periodic or not) could be constructed from an initializer_list.

I am not sure it ever comes up outside of unit tests, but if it is easy or comes as a bonus with a change that generally improves the interface, sure.

2. Homogenize the behavior and messages for construction exceptions:

Cubical cub1({2, 2}, {1, 2, 3, 4, 5});
// Error in constructor Bitmap_cubical_complex_base ( std::vector<unsigned> sizes_in_following_directions, std::vector<T> top_dimensional_cells ). Number of top dimensional elements that follow from sizes_in_following_directions vector is different from the size of top_dimensional_cells vector.
// terminate called after throwing an instance of 'std::invalid_argument'
//   what():  Error in constructor Bitmap_cubical_complex_base( std::vector<unsigned> sizes_in_following_directions,std::vector<T> top_dimensional_cells ). Number of top dimensional elements that follow from sizes_in_following_directions vector is different from the size of top_dimensional_cells vector.
// Aborted (core dumped)

I guess the duplicated message is because some implementations used not to show the exception message, but I don't think that's the case anymore, so we could remove the cerr version.

Here, the code to detect the exception is duplicated. The code could be factorized and the message could be more generic.

If we change the message, it could be nice to include the actual sizes in it, something like "For sizes {x,y,z} we expect a buffer of length n1 but got one of length n2".

Note that this behavior is not the same for the periodic code:

Indeed, adding a check shouldn't be hard.

3. Constructors for periodic/non-periodic

Cubical cub1({2, 2}, {1, 2, 3, 4}, {true, false});  // construct a non-periodic cubical complex

An assertion/exception would be in order here. I don't know if disabling this constructor would cause problems or be worth it.

Periodic_cubical pcub2({2, 2}, {1, 2, 3, 4});  // construct a periodic cubical complex with no periodic direction - A non-periodic would be faster

Here, we could make the compilation fails in these cases.

Failing to compile feels a bit strong. Yes, the non-periodic case would be a bit faster (probably not by a huge factor), but instantiating a single code for all cases can reduce binary code size if you care more about that than performance, keeping it allowed enables us to compare the 2 versions.

With a level of indirection, we could make new interfaces for cubical that is using the correct class in function of the parameters given by the user.

Yes. I am reluctant to do that now when we know that this code is rather slow and a new one may be written eventually, but it is a possibility. For the non-periodic case, the python sklearn interface already dispatches to different code depending on the arguments.

5. Nanobind ndarray does not provide `.begin()`, nor  `.end()` methods, it only provides `.data()` and  `.size()` methods, which, that if we do not want to copy data structures, cubical should provide also constructor like:

Or we can wrap the ndarray in a std::span (or some other range/view object) that can be constructed from pointer and size (one constructor for span checks if its argument supports .data() and .size()) and provides begin+end.

  template<class DimensionIterator, class CellIterator>
  Bitmap_cubical_complex(const DimensionIterator dim_begin, const DimensionIterator dim_end,
                         const CellIterator cell_begin, const CellIterator cell_end,
                         bool input_top_cells = true)

I'd rather stick to range-like interfaces, unless it causes too much trouble.