Skip to content

Remove blas from linfa clustering #257

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

oojo12
Copy link
Contributor

@oojo12 oojo12 commented Oct 31, 2022

Attempt to remove blas support from linfa-clustering per #255

@oojo12
Copy link
Contributor Author

oojo12 commented Oct 31, 2022

Should blas testing also be removed from CI for linfa-clustering?

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Base: 39.17% // Head: 39.24% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (5ac003e) compared to base (1f260de).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   39.17%   39.24%   +0.07%     
==========================================
  Files          92       92              
  Lines        6096     6092       -4     
==========================================
+ Hits         2388     2391       +3     
+ Misses       3708     3701       -7     
Impacted Files Coverage Δ
...linfa-clustering/src/gaussian_mixture/algorithm.rs 44.10% <ø> (+0.88%) ⬆️
algorithms/linfa-tsne/src/hyperparams.rs 16.27% <0.00%> (-2.33%) ⬇️
algorithms/linfa-linear/src/glm/mod.rs 52.77% <0.00%> (ø)
...rithms/linfa-trees/src/decision_trees/algorithm.rs 37.05% <0.00%> (+1.78%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@YuhanLiin
Copy link
Collaborator

You need to update the README as well. Also remove linfa-clustering from the CI.

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 1, 2022

done @YuhanLiin

@@ -24,8 +24,7 @@ Implementation choices, algorithmic details and a tutorial can be found
**WARNING:** Currently the Approximated DBSCAN implementation is slower than the normal DBSCAN implementation. Therefore DBSCAN should always be used over Approximated DBSCAN.

## BLAS/Lapack backend

See [this section](../../README.md#blaslapack-backend) to enable an external BLAS/LAPACK backend.
We found that the pure Rust implementation maintained similar performance to the BLAS/LAPACK version. Thus, to reduce code complexity BLAS support has been removed for this module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mention this PR here as well

@YuhanLiin YuhanLiin merged commit c52cdb5 into rust-ml:master Nov 1, 2022
@oojo12 oojo12 deleted the remove-blas-from-linfa-clustering branch November 5, 2022 18:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants