Skip to content

fix: Fix new clippy warnings #79

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

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

morenol
Copy link
Collaborator

@morenol morenol commented Feb 15, 2021

This fixes the clippy warnings added in the new version of clippy

@morenol
Copy link
Collaborator Author

morenol commented Feb 15, 2021

There is still an error with this clippy::suspicious-operation-groupings. Could you check that @VolodymyrOrlov? It looks to me that we can ignore that warning

VolodymyrOrlov
VolodymyrOrlov previously approved these changes Feb 16, 2021
Copy link
Collaborator

@VolodymyrOrlov VolodymyrOrlov left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -265,7 +266,7 @@ pub trait BaseVector<T: RealNumber>: Clone + Debug {
sum += xi * xi;
}
mu /= div;
sum / div - mu * mu
sum / div - mu.powi(2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering what is faster in Rust, mu * mu or mu.powi(2)? I always thought that multiplication is faster but that might not be true in Rust. This might be a small thing, but if repeated many times the difference will accumulate to a significant value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@VolodymyrOrlov
Copy link
Collaborator

There is still an error with this clippy::suspicious-operation-groupings. Could you check that @VolodymyrOrlov? It looks to me that we can ignore that warning

Agree, the warnings I see do not make any sense. The logic that is highlighted by the lint is correct

@morenol morenol marked this pull request as ready for review February 16, 2021 17:55
@codecov-io
Copy link

Codecov Report

Merging #79 (4f1c7ff) into development (745d0b5) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development      #79      +/-   ##
===============================================
- Coverage        84.17%   84.09%   -0.08%     
===============================================
  Files               78       78              
  Lines             8081     8080       -1     
===============================================
- Hits              6802     6795       -7     
- Misses            1279     1285       +6     
Impacted Files Coverage Δ
src/optimization/first_order/lbfgs.rs 92.85% <ø> (ø)
src/linalg/mod.rs 53.98% <100.00%> (ø)
src/linalg/stats.rs 95.65% <100.00%> (ø)
src/linear/lasso_optimizer.rs 94.11% <100.00%> (ø)
src/algorithm/sort/quick_sort.rs 91.35% <0.00%> (-1.24%) ⬇️
src/naive_bayes/gaussian.rs 83.15% <0.00%> (-1.06%) ⬇️
src/ensemble/random_forest_classifier.rs 71.15% <0.00%> (-0.97%) ⬇️
src/model_selection/kfold.rs 86.48% <0.00%> (-0.91%) ⬇️
src/linalg/naive/dense_matrix.rs 79.55% <0.00%> (-0.30%) ⬇️
src/svm/svc.rs 89.62% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 745d0b5...4f1c7ff. Read the comment docs.

@morenol
Copy link
Collaborator Author

morenol commented Feb 16, 2021

Merging because it already was approved

@morenol morenol merged commit 4af6987 into smartcorelib:development Feb 16, 2021
@morenol morenol deleted the lmm/clippy_warnings branch February 16, 2021 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants