Skip to content
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

update criterion settings #276

Merged
merged 31 commits into from
Nov 17, 2022
Merged

Conversation

oojo12
Copy link
Contributor

@oojo12 oojo12 commented Nov 11, 2022

This PR updates the criterion settings for our benched algorithms. Over the course of a day or a few days I will update this description with the timing stats from before and after changes for each algorithm. Unless, it is okay to just do it for a few then I'll do that instead

linfa-ica

Old - 1min 39secs
New - 3min 38secs

linfa-pls

Old - 4mins 16secs
New - 9mins 42secs

linfa-linear

Old - 1min 29secs
New - 3min 37secs

linfa-nn

Old - 3mins 43secs
New - 8mins 57secs

linfa-clustering

k-means:

Old - 4min 6secs
New - 7mins 34secs

gaussian_mixture

Old - 51secs
New - 2mins 5secs

dbscan

Old - 1min 14secs
New - 2mins 50secs

appx_db_scan

Old - 47secs
New - 1min 27secs

linfa-ftrl

Old - 1min 43secs
New - 3min 57secs

linfa-trees

Old - 55secs
New - 2min 38secs

@oojo12 oojo12 changed the title [WIP] update criterion settings update criterion settings Nov 11, 2022
@YuhanLiin
Copy link
Collaborator

Should we refactor the benchmark configs to a common location? Doing it will be a bit involved, since we'll have to add a new benchmark feature to linfa and put the benchmark configs under that feature.

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 12, 2022

Yeah an importable default would be nice for others and might reduce barriers to contributing benchmarks.

@YuhanLiin
Copy link
Collaborator

Also can you remove the (1000, 5) param for linfa-pls? It takes super long currently.

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022

Codecov Report

Base: 38.59% // Head: 38.44% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (cc0b235) compared to base (11c4d5b).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head cc0b235 differs from pull request most recent head a4de64e. Consider uploading reports for the commit a4de64e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   38.59%   38.44%   -0.16%     
==========================================
  Files          93       95       +2     
  Lines        6223     6227       +4     
==========================================
- Hits         2402     2394       -8     
- Misses       3821     3833      +12     
Impacted Files Coverage Δ
algorithms/linfa-nn/tests/nn.rs 75.60% <0.00%> (ø)
src/benchmarks/mod.rs 0.00% <0.00%> (ø)
src/lib.rs 0.00% <0.00%> (ø)
...rithms/linfa-trees/src/decision_trees/algorithm.rs 37.94% <0.00%> (-1.79%) ⬇️
src/composing/platt_scaling.rs 28.69% <0.00%> (-1.74%) ⬇️
src/dataset/impl_dataset.rs 42.61% <0.00%> (-1.04%) ⬇️
src/metrics_classification.rs 37.97% <0.00%> (+0.63%) ⬆️

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.

@oojo12 oojo12 marked this pull request as draft November 13, 2022 14:07
@oojo12
Copy link
Contributor Author

oojo12 commented Nov 13, 2022

When I find more time to work on fixing this I will do the above and add a configuration for profiling with pprof. In the spirit of helping out with the investigation related issues.

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 14, 2022

@YuhanLiin how do you add a feature? I tried unsuccessfully and couldn't find good docs on the subject.

@YuhanLiin
Copy link
Collaborator

Docs are here. You need to add the feature to the [features] section of Cargo.toml and introduce Criterion as an optional dependency, then have the new feature include Critierion.

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 16, 2022

Got the feature in but now there is a peculiar failure for the ubuntu builds regard the tests for linfa-nn. Not sure what's prompting this error as I haven't adjusted the source code. This failure appears on this branch but is not evident when running the same check from wsl on the master branch

cargo check --workspace --all-targets

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 16, 2022

assert_eq!(out, Vec::<ArrayBase<ViewRepr<&f64>, Dim<[usize; 1]>>>::new()) this line fixes the error and seems to work from my local testing. How do you feel about me updating the test?

Actually, pushing if it isn't desirable, we can always revert.

src/benchmarks/mod.rs Outdated Show resolved Hide resolved
src/benchmarks/config.rs Outdated Show resolved Hide resolved
CONTRIBUTE.md Outdated Show resolved Hide resolved
@YuhanLiin
Copy link
Collaborator

assert_eq!(out, Vec::<ArrayBase<ViewRepr<&f64>, Dim<[usize; 1]>>>::new()) this line fixes the error and seems to work from my local testing. How do you feel about me updating the test?

Actually, pushing if it isn't desirable, we can always revert.

Try ArrayView1<&f64> instead of ArrayBase<_>

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 16, 2022

ArrayView1<&f64>

no dice with Vec::<ArrayView1<&f64>, Dim<[usize; 1]>>::new() instead of Vec::<ArrayBase<ViewRepr<&f64>, Dim<[usize; 1]>>>::new()

@YuhanLiin
Copy link
Collaborator

Try Vec::<ArrayView1<&f64>>

@oojo12
Copy link
Contributor Author

oojo12 commented Nov 16, 2022

Try Vec::<ArrayView1<&f64>>

no dice

@oojo12 oojo12 marked this pull request as ready for review November 16, 2022 05:38
CONTRIBUTE.md Outdated Show resolved Hide resolved
CONTRIBUTE.md Outdated Show resolved Hide resolved
@YuhanLiin YuhanLiin merged commit 0a067bd into rust-ml:master Nov 17, 2022
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