Repository navigation
Add stopping criterion sample count - #341
oleksandr-pavlyk merged 2 commits into
Conversation
e8d7340 to
5f15bd6
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
suggestion: WalkthroughAdds a "sample-count" stopping criterion with a Changessample-count stopping criterion
Assessment against linked issues
Suggested reviewers
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
testing/option_parser.cu (1)
1385-1423: ⚡ Quick winsuggestion: add one regression that sets
--target-samplesglobally before multiple--benchmarkentries and then overrides it for one benchmark, then assert each state gets the expected scoped value. This guards the documented “most recent benchmark or all-before-benchmark” behavior.As per coding guidelines
testing/**/*: focus on whether tests cover observable behavior and remain deterministic.testing/sample_count_criterion.cu (1)
24-55: ⚡ Quick winsuggestion: add boundary cases for
target-samples=1and a non-positive value to lock down criterion semantics at limits and prevent silent behavior drift.As per coding guidelines
testing/**/*: focus on whether tests cover observable behavior and remain deterministic.nvbench/detail/sample_count_criterion.cxx (1)
28-35: ⚡ Quick winimportant: validate
target-samples > 0in initialization (or clamp to 1) before using it indo_is_finished(). Without a guard, non-positive values can make the criterion complete immediately.Based on learnings
--target-samples <count>is documented as a sample count used to stop after at least that many samples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f6f7214-dda4-456b-a3fb-7e0d7a3bb46b
📒 Files selected for processing (15)
docs/cli_help.mdnvbench/CMakeLists.txtnvbench/criterion_manager.cuhnvbench/criterion_manager.cxxnvbench/detail/measure_cold.cunvbench/detail/measure_cpu_only.cxxnvbench/detail/measure_hot.cuhnvbench/detail/sample_count_criterion.cuhnvbench/detail/sample_count_criterion.cxxnvbench/detail/stdrel_criterion.cxxtesting/CMakeLists.txttesting/criterion_manager.cutesting/option_parser.cutesting/sample_count_criterion.cutesting/stdrel_criterion.cu
--stopping-criterion sample-count --target-samples 100 would stop once max(--min-samples, --target-samples) samples are collected
5f15bd6 to
66c7561
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testing/stdrel_criterion.cu (1)
81-94: ⚡ Quick winsuggestion: extend this test to also assert the boundary transition in the same setup (unfinished at 4, finished after the 5th sample) so it validates both sides of the sample-threshold behavior directly.
As per coding guidelines: `testing/**/*`: Focus on whether tests cover observable behavior and remain deterministic.patch
void test_stdrel_needs_enough_samples() { nvbench::criterion_params params; params.set_float64("min-time", 0.0); nvbench::detail::stdrel_criterion criterion; criterion.initialize(params); for (int i = 0; i < 4; ++i) { criterion.add_measurement(42.0); } ASSERT(!criterion.is_finished()); + + criterion.add_measurement(42.0); + ASSERT(criterion.is_finished()); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66f9598f-ab4e-418c-86c4-10a4667d748b
📒 Files selected for processing (15)
docs/cli_help.mdnvbench/CMakeLists.txtnvbench/criterion_manager.cuhnvbench/criterion_manager.cxxnvbench/detail/measure_cold.cunvbench/detail/measure_cpu_only.cxxnvbench/detail/measure_hot.cuhnvbench/detail/sample_count_criterion.cuhnvbench/detail/sample_count_criterion.cxxnvbench/detail/stdrel_criterion.cxxtesting/CMakeLists.txttesting/criterion_manager.cutesting/option_parser.cutesting/sample_count_criterion.cutesting/stdrel_criterion.cu
This PR builds on top of #338, so it should be merged after #338 is merged.Closes #188
This PR add stopping criterion sample-count with target-samples integral parameter, which allows users to collect deterministic number of samples.
Together with
--warmup-runs <count>this permits users to replicate their prior art of running fixed warm-up count and fixed timed runs, but still benefit from NVBench's benchmark reporting facilities, and more accurate GPU timing.The sample-count stopping criterion is essentially the "fixed" custom stopping criterion from
examples/custom_criterion.cu, but interaction with--min-sampleshas changed.Previously, stopping criterion was applied once
m_total_samples > m_min_samples, so running./build/bin/nvbench.example.cpp20.custom_criterion --min-samples 50would collect 51 samples.This PR replaced that condition with
m_total_samples >= m_min_samplesinstead.do_is_finished()method to ensurem_noise_tracker.back()is safe to call.The custom_criterion.cu example needs to be changed to implement an alternative stopping criterion in the future.