-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [las] spanner rank one determinant update implementation #4090
Conversation
float max_volume = -1.0f; | ||
uint64_t U_rid{}; | ||
const Eigen::RowVectorXf original_row = X.row(X_rid); | ||
|
||
for (auto i = 0; i < U.rows(); ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't need to check all U rows, since some rows are already used in X, and using a duplicate row will make the determinant zero, and thereby not choosing it, but we'll pay the determinant computation cost. One potential optimization is to pass in the bit vector to eliminate already used rows, instead of trying all U rows, which might be more useful when d is big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll benchmark this and see if it should be implemented next
… on action id and not preds index in the pred vector)
…it into las_spanner_speedup
vowpalwabbit/core/src/reductions/cb/details/large_action_space.h
Outdated
Show resolved
Hide resolved
for (auto it = preds.begin(); it != preds.end(); it++) | ||
{ | ||
if (!_spanner_state._spanner_bitvec[index]) { preds.erase(it--); } | ||
index++; | ||
if (!_spanner_state._spanner_bitvec[it->action]) { preds.erase(it--); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it--
from begin() is undefined, although it later increases. Better make it safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced
vowpalwabbit/core/src/reductions/cb/cb_explore_adf_large_action_space.cc
Outdated
Show resolved
Hide resolved
I'll split the spanner implementations in next PR and do some cleanup |
based on Sherman–Morrison formula and matrix determinant lemma
might templetize the two spanners later