feat: large action spaces truncated randomized SVD #3899

olgavrou · 2022-04-28T17:34:21Z

qr decomposition and randomized svd steps
also generating the sparse action weight matrix A for testing
unit tests added for the randomized svd steps

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

jackgerrits · 2022-05-02T15:52:24Z

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

+    if (_all->weights.sparse)
+    {
+      triplet_constructor<sparse_parameters> w(_all->weights.sparse_weights, row_index, _triplets, max_non_zero_col);
+      GD::foreach_feature<float, float, no_op, triplet_constructor<sparse_parameters>>(w, _all->ignore_some_linear,


Rather than hiding the construction logic in the operator[] it would make more sense to do the insertion in the function that is called per weight. This would also migrate more cleanly to if this function where to be able to use a lambda instead of context + fptr

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

…n_space.cc Co-authored-by: Jack Gerrits <jackgerrits@users.noreply.github.com>

…into QR_decomposition

jackgerrits

I can't really comment on the linear algebra, but there are a few comments for the rest

jackgerrits · 2022-05-16T18:43:23Z

vowpalwabbit/core/include/vw/core/qr_decomposition.h

+#include <Eigen/Dense>
+#include <Eigen/SparseCore>


Forward declare instead of including the headers to improve build time and requiring the dependency for this header

Given that this is an external library the convention is to just include it and respect its interface (also see google style guide)

We are only using the types via references in this header so there is no need to include it all here.

We are explicitly using forward declarations as much as possible in VW in a attempt to improve compile time and reduce header interdependencies.

EDIT: Makes complete sense for a external type, especially this non-trivial one

Synced offline, this is an external library and the types that are included are complicated (templated with default template args, etc) so we will respect the boundaries by including

jackgerrits · 2022-05-16T18:47:41Z

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

 template <typename WeightsT>
-struct LazyGaussianDotProduct
+struct A_triplet_constructor


Can you update to match naming conventions? If A is a thing can it be named something more descriptive?

Following the redsvd implementation for randomized truncated SVD from here. Keeping the equivalent matrix names is very useful, especially since this is still under construction and some steps/matrixes might be removed in the future

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

jackgerrits · 2022-05-16T18:49:11Z

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

+};
+
+template <typename WeightsT>
+struct Y_triplet_constructor


Same comment about naming

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

jackgerrits · 2022-05-16T18:53:50Z

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

+            _all->_generate_interactions_object_cache);
+      }
+
+      B(row_index, col) = dot_product;


What is B? Can it please be named something more descriptive

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc

jackgerrits · 2022-05-16T18:59:33Z

vowpalwabbit/core/src/reductions/cb/details/large_action_space.h

+  Eigen::MatrixXf B;
+  Eigen::MatrixXf Z;
+  Eigen::MatrixXf U;
+  Eigen::VectorXf _S;


Why are some prefixed with _ and not others?

they are currently only used in the unit tests, we don't need them otherwise, but unit testing can not happen without them

Perhaps specify that fact either in the variable name or a comment?

fair, added comments

olgavrou added 7 commits April 28, 2022 10:36

QR decomposition and create original A matrix

f1a89a1

cleanup and testing

63ca8ab

remove un-needed code

4778185

Merge branch 'master' into QR_decomposition

c1c7ab6

formatting

6c818f1

add svd step too

75c0e16

Merge branch 'master' into QR_decomposition

faa9489

olgavrou changed the title ~~feat: large action spaces QR decomposition~~ feat: large action spaces QR decomposition and SVD steps Apr 29, 2022

jackgerrits reviewed May 2, 2022

View reviewed changes

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc Outdated Show resolved Hide resolved

jackgerrits reviewed May 2, 2022

View reviewed changes

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc Outdated Show resolved Hide resolved

jackgerrits reviewed May 2, 2022

View reviewed changes

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc Outdated Show resolved Hide resolved

jackgerrits reviewed May 2, 2022

View reviewed changes

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc Outdated Show resolved Hide resolved

jackgerrits reviewed May 2, 2022

View reviewed changes

vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc Outdated Show resolved Hide resolved

olgavrou and others added 16 commits May 2, 2022 12:06

Merge branch 'master' into QR_decomposition

2d69e5a

Merge branch 'master' into QR_decomposition

ef70529

Update vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_actio…

c1e48a3

…n_space.cc Co-authored-by: Jack Gerrits <jackgerrits@users.noreply.github.com>

try different approach

1d60463

add first unit test for SVD step

994ddf0

proper test

2d7b8ae

cleanup and licencing

126c4fa

Merge branch 'master' into QR_decomposition

71f373d

formatting

0298f5c

add some test documentation and cleanup

6148fea

Merge branch 'master' into QR_decomposition

e9e5839

boost check small

f5becd6

Merge branch 'QR_decomposition' of github.com:olgavrou/vowpal_wabbit …

0782f10

…into QR_decomposition

..

127ebbf

set matrixes to zero when resizing

d5959ff

PR feedback

45b5ffb

olgavrou added 4 commits May 16, 2022 12:57

cleanup

f61f9c9

Merge branch 'master' into QR_decomposition

aee43a4

formatting

49a6286

using eigen index

654cdf2

olgavrou changed the title ~~feat: large action spaces QR decomposition and SVD steps~~ feat: large action spaces truncated randomized SVD May 16, 2022

add redsvd implementation note

348b080

jackgerrits requested changes May 16, 2022

View reviewed changes

olgavrou added 3 commits May 16, 2022 15:27

styling pr comments and small fixes

430024d

formatting

3633ce4

use existing action score comparator

ec7121f

olgavrou requested a review from jackgerrits May 17, 2022 13:43

Merge branch 'master' into QR_decomposition

e1ae6a3

jackgerrits approved these changes May 18, 2022

View reviewed changes

olgavrou merged commit 231dcdf into VowpalWabbit:master May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: large action spaces truncated randomized SVD #3899

feat: large action spaces truncated randomized SVD #3899

olgavrou commented Apr 28, 2022 •

edited

jackgerrits May 2, 2022

olgavrou May 16, 2022

jackgerrits left a comment

jackgerrits May 16, 2022

olgavrou May 16, 2022

jackgerrits May 16, 2022 •

edited

olgavrou May 16, 2022

jackgerrits May 16, 2022

olgavrou May 16, 2022

jackgerrits May 16, 2022

jackgerrits May 16, 2022

jackgerrits May 16, 2022

olgavrou May 16, 2022 •

edited

jackgerrits May 16, 2022

olgavrou May 16, 2022

		#include <Eigen/Dense>
		#include <Eigen/SparseCore>

feat: large action spaces truncated randomized SVD #3899

feat: large action spaces truncated randomized SVD #3899

Conversation

olgavrou commented Apr 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackgerrits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackgerrits May 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olgavrou May 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olgavrou commented Apr 28, 2022 •

edited

jackgerrits May 16, 2022 •

edited

olgavrou May 16, 2022 •

edited