-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Coot S01E02 #3600
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
Coot S01E02 #3600
Conversation
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
I thought for This should pass for |
This reverts commit e082b90.
|
maybe someone needs to propose this to the C++ committee, to change ADL to ATDL 😛 |
rcurtin
left a comment
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.
The changes look good to me (let's see if the tests pass, I think they should). The main comment I have is tedious: lots of the lines can be reflowed for easier readability now, since they are shorter.
| void AddType<MatType>::Forward(const MatType& input, MatType& output) | ||
| { | ||
| output = input + arma::repmat(arma::vectorise(weights), 1, input.n_cols); | ||
| output = input + repmat(arma::vectorise(weights), 1, input.n_cols); |
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.
You could change arma::vectorise to vectorise too, or in another PR, up to you. 👍
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.
in another PR, will make them simple short and fast
|
I suppose maybe we will need to make a |
Co-authored-by: Ryan Curtin <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@mlpack-jenkins test this please |
rcurtin
left a comment
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.
Looks good. I found a bunch of places where we could still shorten lines (and one case where the line is too long). But, those are all small style concerns. Functionality-wise it looks great to me. 👍
src/mlpack/methods/softmax_regression/softmax_regression_function_impl.hpp
Outdated
Show resolved
Hide resolved
src/mlpack/methods/softmax_regression/softmax_regression_impl.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
…ion_impl.hpp Co-authored-by: Ryan Curtin <[email protected]>
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.
Second approval provided automatically after 24 hours. 👍
This could be merged fast.
arma::repmat -> repmatarma::randu -> randurepmatfunction name only exist in Matlab, so no harm in removing arma,Opencv has the same
randuname so it could be tricky, especially if someone is doing object detection, unless if they usecv::randuin their code.