- marked as minor
STL compatibility
This would presumably be somewhat major, but it would be great if the blaze library would be more compatible with the STL than it currently seems to be.
First a disclaimer: I am a very new user who saw the talk from CppCon16, and was inspired to try to migrate an existing code base I have to blaze to see if this could help with performance. I have tried my best to find whatever information I require from the documentation, but there is of course the chance I missed something. However, as I was trying to adapt my code I ran into a couple of issues that would have had a cleaner solution if the library was STL compatible, and had the "expected" interface.
There are at the moment two situations in which blaze handles unintuitively form my perspective:
- The begin() and end() functions shouldn't take an index. It would be better if row and column iterators for matrices were a separate thing from a pure data iterator. And that these had separate begin and end functions. At the moment, if I want to use std::for_each or std::transform with a matrix I think I'd have to call
std::for_each(mat.data(), mat.data() + mat.capacity(), [](){...});
- Would be good if one had a forEach alternative that worked in place instead of returning a new matrix, but I would assume that
mat = blaze::forEach(mat, [](){...});
has the desired effect.
These are simply my experiences trying to adapt a code written in armadillo (and then later in Eigen3) to blaze, and it would be interesting to hear your thoughts on this, or possible how you think this should be handled.
Comments (4)
-
reporter -
-
assigned issue to
-
assigned issue to
-
- changed status to wontfix
Hi Jonas!
Thanks a lot for raising this issue. STL compatibility is indeed very important for us and we try hard to adhere to the existing C++ coding conventions. However, we unfortunately cannot change the two problems you describe. Allow me to explain:
begin()/end()
As stated, STL compatibility is very important for us. However, performance tops STL compatibility. In the case of matrices, providing a
begin()
andend()
member function that allows you to traverse the entire range of elements would prevent certain optimizations such as for instance padding, which is extremely important for the performance of tiny and small matrices.The solution that you describe for traversing the entire range of elements using the
data()
function is unfortunately not a solution:std::for_each(mat.data(), mat.data() + mat.capacity(), [](){...});
In this case you also write to the padding elements, which may cause undefined behavior. As stated in the documentation of the
data()
function, ...... you can NOT assume that all matrix elements lie adjacent to each other! The dynamic matrix may use techniques such as padding to improve the alignment of the data. Whereas the number of elements within a row/column are given by the rows() and columns() member functions, respectively, the total number of elements including padding is given by the spacing() member function.
forEach()
The solution you propose to change a matrix in place is correct:
mat = blaze::forEach(mat, [](){...});
Since this interface can be universally be used to assign a matrix to itself or to assign to other matrices, we don't see the advantage to add a second interface for in-place transformations. In face, we try to be very consistent with our interfaces. This means that you can also do the following:
mat = blaze::trans( mat ); // In-place transposition of a matrix mat = blaze::ctrans( mat ); // In-place conjugate transposition of a matrix mat = blaze::inv( mat ); // In-place transposition of a matrix
Admittedly, there is also
transpose()
andctranspose()
member functions in the matrix classes, which perform an in-place transposition. However, just as in case of the STL, where for instancestd::set
providesfind()
,lower_bound()
, andupper_bound()
member functions for performance reasons, Blaze provides these functions to accommodate for the different data structures. The preferred interface is the free functions.Again, thanks a lot for raising this issue.
Best regards,
Klaus!
-
Issue
#178was marked as a duplicate of this issue. - Log in to comment