Skip to content

S01E08 Remove arma:: element wise functions and join_cols and join_rows` #3610

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 35 commits into from
Jan 28, 2024
Merged

S01E08 Remove arma:: element wise functions and join_cols and join_rows` #3610

merged 35 commits into from
Jan 28, 2024

Conversation

shrit
Copy link
Member

@shrit shrit commented Jan 19, 2024

Hopefully this passes on Windows

shrit added 15 commits January 19, 2024 13:03
@shrit
Change from arma::exp to exp using ADL
f04404b 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::exp to exp using adl, in core and tests
16c4c73 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::sum to sum using ADL in methods
6ab2063 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::sum to sum using adl in core/
8854303 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::sum to sum using adl in tests
9b3384d 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::log to log using adl in methods
395f01e 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Chage arma::log to log using adl in tests/ core/ and methods/
ed31818 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::log to log using adl in methods
f896b6e 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::sqrt to sqrt in core tests using adl
9fc79b2 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::sign to sign using adl in methods tests
8d6f25b 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::pow to pow using adl in methods
8e3c464 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::pow to pow using adl in tests and core
37f0c88 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::join_cols to join_cols using adl
7439bdf 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::join_cols to join_cols in tests
a1e2dd1 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Change arma::join_rows to join_rows
fb88981 
Signed-off-by: Omar Shrit <[email protected]>
@shrit shrit requested a review from rcurtin January 19, 2024 13:13
@rcurtin
Copy link
Member

rcurtin commented Jan 20, 2024

The test failure on OS X looks unrelated (it's that RADICAL test failure I haven't been able to pin down). 👍

@shrit
Copy link
Member Author

shrit commented Jan 22, 2024

Okay, I see, do I restart the tests?

@rcurtin
Copy link
Member

rcurtin commented Jan 23, 2024

No need, I know your changes did not cause that.

rcurtin
rcurtin approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that there wasn't a problem on Windows with arma::pow -> pow, since there is also std::pow. But, it works, so, it looks good to me. There are a lot of places where the lines were shorter; I know it is a really pedantic comment to point it out over and over again (and also tedious), so I did my best to provide easy-to-click suggestions, but there were some places where Github wouldn't allow me to do that.

src/mlpack/methods/ann/dists/normal_distribution_impl.hpp
@@ -44,7 +44,7 @@ template<typename DataType>
DataType NormalDistribution<DataType>::LogProbability(
const DataType& observation) const
{
const DataType v1 = arma::log(sigma) + std::log(std::sqrt(2 * M_PI));
const DataType v1 = log(sigma) + std::log(std::sqrt(2 * M_PI));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to make this all ADL, you could remove the std:: too, but up to you; to me it doesn't make a difference either way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, I agree, let us for once make the Windows compiler happy

shrit and others added 15 commits January 27, 2024 16:28
@shrit @rcurtin
Update src/mlpack/tests/decision_tree_regressor_test.cpp
4357324 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/tests/decision_tree_regressor_test.cpp
2214660 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/tests/decision_tree_regressor_test.cpp
59ba005 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/tests/softmax_regression_test.cpp
ed6d5dd 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/tests/softmax_regression_test.cpp
eff0a88 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/ann/layer/not_adapted/virtual_batch_norm_im…
e32a9f6 
…pl.hpp

Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/tests/decision_tree_regressor_test.cpp
97def6a 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/ann/layer/radial_basis_function_impl.hpp
83166f0 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/ann/layer/radial_basis_function_impl.hpp
351e01a 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/ann/layer/softmax_impl.hpp
f96d31a 
Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/ann/loss_functions/mean_squared_logarithmic…
71352f1 
…_error_impl.hpp

Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/softmax_regression/softmax_regression_funct…
2d24b96 
…ion_impl.hpp

Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/bayesian_linear_regression/bayesian_linear_…
d7b96ba 
…regression_impl.hpp

Co-authored-by: Ryan Curtin <[email protected]>
@shrit @rcurtin
Update src/mlpack/methods/bayesian_linear_regression/bayesian_linear_…
90e3ade 
…regression_impl.hpp

Co-authored-by: Ryan Curtin <[email protected]>
@shrit
Fix all remaining style errors
496101a 
Signed-off-by: Omar Shrit <[email protected]>
@shrit
Copy link
Member Author

shrit commented Jan 27, 2024

Thanks, I had to click on all the suggestions, I'm so sorry if you have received 35 emails, it is easier rather than going through each one of them with vim
Yes, I am surprised that this was not an issue on Windows, but if it is passed then it is okay for now

mlpack-bot[bot]
mlpack-bot bot approved these changes Jan 27, 2024
View reviewed changes
Copy link

@mlpack-bot mlpack-bot bot left a 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. 👍

@shrit shrit merged commit 15f7326 into mlpack:master Jan 28, 2024
@rcurtin
Copy link
Member

rcurtin commented Jan 28, 2024

Thanks, I had to click on all the suggestions, I'm so sorry if you have received 35 emails, it is easier rather than going through each one of them with vim

No worries, easy enough to quickly go through the emails and see what happened 👍

This was referenced May 26, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcurtin rcurtin rcurtin approved these changes

@mlpack-bot mlpack-bot[bot] mlpack-bot[bot] approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shrit @rcurtin