S01E05 Convert all the init methods from arma::mat to MatType #3607
Conversation
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Nice, another easy PR to review. Everything here looks good. 👍
| template<typename MatType> | ||
| struct GetColType | ||
| { | ||
| typedef arma::Row<typename MatType::elem_type> type; |
There was a problem hiding this comment.
You are welcome, it is my fault, I reviewed this and it was good for me 😵💫
| ColType s; | ||
|
|
||
| arma::svd_econ(W, s, V, arma::randu<arma::Mat<eT> >(rows, cols)); | ||
| svd_econ(W, s, V, arma::randu<MatType>(rows, cols)); |
There was a problem hiding this comment.
Note that Bandicoot doesn't currently have svd_econ(), but we can fix that later of course. 👍
There was a problem hiding this comment.
Yes, all of these need to be handled later, I will add this to the list in bandicoot we will do it in the next version.
I do not think a lot of us are using orthogonal initialisation for now
| { | ||
| arma::Row<eT> b = s * arma::sqrt(3 / (W.n_rows * dataSum)); | ||
| typedef typename GetRowType<MatType>::type RowType; | ||
| RowType b = s * arma::sqrt(3 / (W.n_rows * dataSum)); |
There was a problem hiding this comment.
I wonder if for most of these implementations we could just call Initialize(W, W.n_rows, W.n_cols). Up to you if you want to take the effort to do that refactoring... and that assumes that the two functions are the same otherwise.
There was a problem hiding this comment.
I thought about it, maybe at the very end, because this will get refactored. We need to get rid of arma::sqrt and create a new safe function, so will see.
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
2 participants
This PR converts all the
arma::Mat<eT>in the init methods toMatType.This should be merged quickly.