[Persistence Matrix] Addition of small vectors as a column type #1154
Conversation
| using STD_naive_vector_column = Naive_vector_column<Master_matrix, std::vector<typename Master_matrix::Matrix_entry*> >; | ||
| template <class Master_matrix> | ||
| using Small_naive_vector_column = | ||
| Naive_vector_column<Master_matrix, boost::container::small_vector<typename Master_matrix::Matrix_entry*, 8> >; |
There was a problem hiding this comment.
I agree. It would be better to have something general here, but I didn't wanted to add an option for that... And 8 was what worked the best in my benchmarks to compute a barcode. Do you see an easy way to give the possibility to customize this value for the user without having to add an option just for this particular case?
There was a problem hiding this comment.
Not "easy": in my opinion, the original issue is that using an enum makes the design not extensible (a user cannot provide their own column type). If you changed the design enough for it to be extensible, then there would be no limitation on parameterizing the column types you provide.
(IIRC, Boost.Graph has a similar limitation with a hard-coded list of possible containers for some data structures)
Without going all the way to an extensible design, using tag classes instead of an enum (i.e. empty classes that are only used with is_same<T,vector_column_tag> or similar) might not require huge changes and could accommodate small_vector_tag<8>.
Anyway, I'll approve this for now, but please make a note somewhere about this (an issue, or a comment in the code, I don't know if you already have a list of potential improvements somewhere).
There was a problem hiding this comment.
We will keep track of this on #1167
| @@ -224,7 +224,13 @@ class Naive_vector_column : public Master_matrix::Row_access_option, | |||
| }; | |||
|
|
|||
| template <class Master_matrix> | |||
| inline Naive_vector_column<Master_matrix>::Naive_vector_column(Column_settings* colSettings) | |||
| using STD_naive_vector_column = Naive_vector_column<Master_matrix, std::vector<typename Master_matrix::Matrix_entry*> >; | |||
There was a problem hiding this comment.
What do you think of Naive_std_vector_column and Naive_small_vector_column, to keep the qualifier of vector next to the word vector?
There was a problem hiding this comment.
Sounds much better! Changed it.
It makes indeed also sense for Vector_column. It's just that Vector_column only works better when a lot of entries are manually cleared inside the column, which for now only happens during vineyard with RU. And in that case, the small vector do not seem to make things much better. At least in multipersistence. And I am wondering a bit, if the class is not indeed useless as you say, as using a set kind of works better in that particular case anyway... |
Replacing
std::vectorwithboost::container::small_vectorinNaive_vector_columnshowed some run time improvements on some particular use cases in multipers. So I added the possibility in the column choices.