Coot 11 Change arma::accu to Accu, and part of arma::max #3619
Conversation
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@shrit @rcurtin @zoq I have concerns about this PR, as well as the adverse cumulative effect of all coot related PRs. The changes are too big for not much benefit, at the considerable cost of potential regressions, increased maintenance burden, as well as complicating the mlpack codebase. I think there might be a better way forward -- see the corresponding issue in the mlpack contributors chat. |
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@rcurtin this is ready for review |
There was a problem hiding this comment.
Nice changes. My comments are all little things... as I go through I often notice little things that are easy enough to fix. I did my best to leave suggestions everywhere so that it's not much work on your end. In either case though even if you don't take the suggestions I think everything is fine. 👍
src/mlpack/core/util/conv_to.hpp
Outdated
| @@ -20,7 +20,7 @@ namespace mlpack { | |||
| * A utility class that based on the data type forwards to `coot::conv_to` or | |||
| * `arma::conv_to`. | |||
| * | |||
| * @tparam OutputType The data type to convert to. | |||
There was a problem hiding this comment.
Technically that should still be @tparam (it is a template parameter). It's a bit of a moot point since we don't use doxygen anymore, but still, I think developers read this inline documentation.
There was a problem hiding this comment.
Aha, that's something new I learned, I thought that this was a typo error.
| arma::vec index = ConvTo<arma::vec>::From(arma::find(Map | ||
| == arma::max(Map))); | ||
| == max(Map))); |
There was a problem hiding this comment.
This one will fit on one line now (sorry, Github won't let me suggest it).
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]>
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]>
Co-authored-by: Ryan Curtin <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@mlpack-jenkins test this please |
I will not more to this one, it is already 143 files.
Will follow up with subsequent PRs