refactor: change benchmark sample sizes to 10k-100k#16
refactor: change benchmark sample sizes to 10k-100k#16Neclow merged 4 commits intosbhattlab:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the benchmark sample sizes for phylo2vec to use fixed multiples of 10,000, resulting in sample sizes ranging from 10k to 100k, and updates the Criterion configuration accordingly.
- Updated SAMPLE_SIZES from a power-of-2 range to a multiplier-based range.
- Converted sample size calculation from 2^i to 10000 * i.
- Increased the Criterion sample size from 10 to 30.
Comments suppressed due to low confidence (1)
phylo2vec/benches/benchmarks/core.rs:75
- [nitpick] Verify that the increased sample size setting for benchmark iterations (30 instead of 10) provides statistically reliable results without causing excessive benchmark run times.
.sample_size(30)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors benchmark tests by changing sample size ranges from powers-of-two to fixed sizes between 10k and 100k, and adjusts the Rust benchmark configuration to use a higher Criterion sample size.
- Updated Python benchmarks to use a BIG_RANGE constant
- Refactored import alias from _phylo2vec_core to core in Python tests
- Modified the Rust benchmark to use a RangeInclusive multiplier with sample sizes scaled by 10,000 and increased Criterion’s sample_size from 10 to 30
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| py-phylo2vec/benchmarks/test_bench.py | Updated sample size range and import alias for benchmarking tests |
| phylo2vec/benches/benchmarks/core.rs | Changed benchmark sample size calculations and adjusted Criterion's sample size |
Comments suppressed due to low confidence (1)
py-phylo2vec/benchmarks/test_bench.py:5
- [nitpick] Consider renaming the alias 'core' to something like 'p2v_core' to better reflect the module's purpose and maintain consistency with previous naming.
from phylo2vec import _phylo2vec_core as core
| @@ -73,7 +76,7 @@ fn bench_to_vector(c: &mut Criterion) { | |||
| criterion_group! { | |||
| name = core; | |||
| config = Criterion::default() | |||
There was a problem hiding this comment.
Add a comment explaining the rationale for increasing the sample_size to 30 to aid future maintainers in understanding the performance trade-offs.
| config = Criterion::default() | |
| config = Criterion::default() | |
| // Set sample_size to 30 to balance statistical accuracy with reasonable execution time. |
No description provided.