r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
199 stars 46 forks source link

Columns support for matrix class #229

Closed alyst closed 3 years ago

alyst commented 3 years ago

This draft PR adds matrix::column class in full analogy to matrix::row class and updates the matrix API so that both rows and columns could be accessed. I just added it because my use-case required column-wise access. I'm not sure about the proposed matrix API, it was just the easiest to implement. Alternatively one could think about matrix::row_vector proxy class and matrix::rows() method that would provide row access (begin/end/[] etc) and matrix::column_vector that would provide symmetric columns access.

I also added the required types to matrix::row::iterator so that it actually conforms to the iterator concept.

alyst commented 3 years ago

The tests are failing because the matrix::row-related API has changed. I will fix the tests once there is a decision how this API should actually look like.

jimhester commented 3 years ago

An alternative interface would be to let the user to supply if they want to use column or row based indexing as part of the matrix constructor, either as a template parameter or an argument.

I think generally users only want to iterator over rows or columns, and very rarely do both with the same matrix. If they do want to use both in the same application they could create two matrix objects pointing at the same underlying data.

alyst commented 3 years ago

@jimhester I've tried to implement your suggestion.

Now the matrix class has the 3rd template parameter, S (how the matrix is sliced), which could be either by_row or by_column. So all fully specialized matrix types are duplicated: there is doubles_row_matrix and doubles_column_matrix. The matrix::row class is replaced by matrix::slice, which uses matrix::slice_xxx() methods to define how it accesses the matrix elements. matrix<V,T,S> is inherited from matrix_slices<S>, where the slice_xxx methods are actually defined (there are matrix_slices<by_row> and matrix_slices<by_column> specializations).

It's been a while since I wrote something in C++, I hope I have not overcomplicated these specialization things.

If this approach looks good, I can start updating the tests.

alyst commented 3 years ago

@jimhester the tests pass now

jimhester commented 3 years ago

Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

Otherwise I think this looks great, thanks for working on it!

alyst commented 3 years ago

@jimhester done

jimhester commented 3 years ago

Thanks again for working on this!

Thanks a bunch!