Broken constructor in matrix adaptors

[Marcin Copik]

I've found two problems with constructors initializing a 2D matrix with values passed in an array.

First is inconsistent syntax, a rather minor issue. Dense DynamicMatrix defines the constructor syntax as (size_t, size_t, T*). This syntax is not followed in adaptors though: HermitianMatrix has (T*, size_t, size_t) or (T*, n). DiagonalMatrix has the same syntax. I think all dense matrix types should follow the same style. Now users (e.g. me) are forced to duplicate template code constructing a Blaze matrix.

A second problem is that those constructors call an unexisting ctor with 4 arguments in parent(DynamicMatrix). It looks like an unfinished refactorization and changing the call to (n, n, ptr) removes compilation errors and everything seems to work fine - at least for HermitianMatrix.

I could easily fix it and create a pull request but I don't know which solution is preferred for 1), if my solution for 2) is correct and how many classes are affected by 2), possibly all adaptors? I'd suggest to enforce an identical syntax for this ctor on all matrix classes and then fix incorrect calls in adaptors.

[Klaus Iglberger]

Hi Marcin!

Thanks for raising this issue. However, the code is correct and works according to specification. Also, all constructors are properly tested in our test suite.

All dense matrices have a constructor that enables a user to initialize it from a given array. Since general matrices can have different numbers of rows and columns, it is necessary to specify two size arguments. For instance:

template< typename Type
        , bool SO >
class DynamicMatrix : public DenseMatrix< DynamicMatrix<Type,SO>, SO >
{
   // ...
   explicit inline DynamicMatrix( size_t m, size_t n, const Type& init );
   // ...
};

Just as the basic matrix types (StaticMatrix, DynamicMatrix, HybridMatrix, CompressedMatrix) all adaptors have a constructor for the same task. However, in constrast to the basic matrix types adaptors always represent square matrices. For that reason, the according constructor in all adaptors only expects a single size argument. For instance:

template< typename MT
        , bool SO >
class HermitianMatrix<MT,SO,true>
   : public DenseMatrix< HermitianMatrix<MT,SO,true>, SO >
{
   // ...
   template< typename Other >
   explicit inline HermitianMatrix( size_t n, const Other* array );
   // ...
};

This design is consistent in all adaptors and in all member functions of adaptors (i.e. one size argument instead of two).

Additionally, all adaptors have four constructors, which enable a user to wrap a CustomMatrix into an adaptor:

template< typename MT
        , bool SO >
class HermitianMatrix<MT,SO,true>
   : public DenseMatrix< HermitianMatrix<MT,SO,true>, SO >
{
   // ...
   explicit inline HermitianMatrix( ElementType* ptr, size_t n );
   explicit inline HermitianMatrix( ElementType* ptr, size_t n, size_t nn );

   template< typename Deleter >
   explicit inline HermitianMatrix( ElementType* ptr, size_t n, Deleter d );

   template< typename Deleter >
   explicit inline HermitianMatrix( ElementType* ptr, size_t n, size_t nn, Deleter d );
   // ...
};

These constructors are only expected to work in combination with a CustomMatrix, which is initialized by passing a pointer to some existing storage. They will not work with any other matrix type. Please note that the constructor for initializing a matrix with a given array expects its pointer parameter as the last argument, whereas all constructors for CustomMatrix expect the pointer parameter as the first argument.

I hope this explains the design and the differences between basic matrices and adaptors.

Best regards,

Klaus!

[Marcin Copik]

@eagle42 Thank you for the explanation. I apologize for not noticing this difference, somehow I did not notice interpret the "custom matrix" in docs as referring to type CustomMatrix. A possible, but not mandatory, improvement here would be to use std::enable_if on those constructors to enable them only when parent type is a Blaze CustomMatrix.