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
Add linalg API to allow adding a vector to diagonal #4087
Add linalg API to allow adding a vector to diagonal #4087
Conversation
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.
minor changes requires, otherwise great!
Well done on digging into this mess :)
@@ -189,6 +190,35 @@ TEST(LinalgBackendEigen, SGMatrix_add_col_vec_in_place) | |||
} | |||
} | |||
|
|||
TEST(LinalgBackendEigen, add_diag_vec) | |||
{ | |||
const float64_t alpha = 0.7; |
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.
could you do this test more explicit, i.e. just write down the matrices by hand (no loops)
Otherwise good.
Also make sure to use EXPECT_THROW
for the case of invalid input arugments (the REQUIRE
)
Finally, test some corner cases (empty matrix doesnt give memory error, etc)
* | ||
* @see linalg::add_diag_vec | ||
*/ | ||
#define BACKEND_GENERIC_ADD_DIAG_VEC(Type, Container) \ |
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 guess I would call this add_diag
rather than add_diag_vec
A add_diag API to linalg to allow adding a vector to diagonal of a matrix.
4fc02bf
to
6693918
Compare
EXPECT_THROW(add_diag(A1, b2), ShogunException); | ||
EXPECT_THROW(add_diag(A2, b1), ShogunException); | ||
EXPECT_THROW(add_diag(A2, b2), ShogunException); | ||
} |
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.
IMHO, I'd shove all add_diag
-related tests into one test unit for the neatness.
Otherwise good to me :)
Good work, thanks! |
As mentioned in #4085 by @karlnapf , we need a API in linalg to allow adding a vector to a matrix's diagonal. This PR adds
add_diag_vec
that performs operationA.diagonal = alpha * A.diagonal + beta * b
. Could you provide some feedback on the API design ?