Use robust centrality dispersion measures#348
Conversation
oleksandr-pavlyk
added
the
release: breaking change
oleksandr-pavlyk
force-pushed
the
use-robust-centrality-dispersion-measures
branch
from
8d1b316 to
6e45072
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe PR replaces conventional mean/standard-deviation statistics with robust quartile and interquartile-range (IQR) metrics across NVBench CPU and GPU timing measurements. A new ChangesRobust statistics implementation
Assessment against linked issues
Suggested labels
Suggested reviewers
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
testing/statistics.cu (2)
90-97: ⚡ Quick winsuggestion: Lines 94-96 only assert non-finite values; this would still pass if behavior regresses to infinity. Assert
std::isnan(actual[i])to lock the empty-input contract explicitly.As per coding guidelines, "testing/**/*: Focus on whether tests cover observable behavior, remain deterministic, handle GPU availability and CUDA version differences correctly, avoid excessive runtime, and exercise install/export/package boundaries where relevant."
56-80: ⚡ Quick winsuggestion: Add a 2-element dataset case (for example,
{10, 20}with percentile50) to pin the rank-rounding rule at half steps and prevent future semantic drift.As per coding guidelines, "testing/**/*: Focus on whether tests cover observable behavior, remain deterministic, handle GPU availability and CUDA version differences correctly, avoid excessive runtime, and exercise install/export/package boundaries where relevant."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d1769301-edcf-4efd-9f48-b076a0f8a23e
📒 Files selected for processing (6)
nvbench/detail/measure_cold.cunvbench/detail/measure_cpu_only.cxxnvbench/detail/statistics.cuhnvbench/detail/stdrel_criterion.cxxpython/scripts/nvbench_compare.pytesting/statistics.cu
| auto &summ = m_state.add_summary("nv/cold/time/cpu/ir/relative"); | ||
| summ.set_string("name", "Noise"); | ||
| summ.set_string("hint", "percentage"); | ||
| summ.set_string("description", | ||
| "Relative interquartile range of isolated kernel execution CPU times"); | ||
| const auto cpu_time_ir = cpu_time_third_quartile - cpu_time_first_quartile; | ||
| const auto cpu_robust_noise = cpu_time_ir / cpu_time_median; | ||
| summ.set_float64("value", cpu_robust_noise); | ||
| } |
There was a problem hiding this comment.
important: Relative IR is computed as IQR / median at Line 312 and Line 417 with no zero/finite guard. For very short or quantized timings, median can be zero and produce inf/nan summaries. Guard both calculations and publish only finite values.
As per coding guidelines, "nvbench/**/*: Focus on benchmark correctness, ... measurement semantics, statistical summaries, and test coverage."
Also applies to: 411-419
Percentiles on empty dataset are NaN, not infinity Add Robust statistics of CPU times to summary Fixed name for nv/cold/time/gpu/q3, corrected value reported for nv/cold/time/gpu/ir/relative Use median and IR to compute location and noise in measure_cold Also in stdrel_criterion, compute noise as IR / median.
1. For JSON files that contains repeated measurements of run-time axis values, make sure that scripts compares corresponding reference entries. If cmp had two states with the same name and ref had two, we would compare measurements for each state in cmp against the first state in ref. Change here introduces counters tracking how many times each particular axis value, and retrieve corresponding entry in ref. Previously, I had ``` | BlockSize | NumBlocks | Ref Time | Ref Noise | Cmp Time | Cmp Noise | Diff | %Diff | Status | |-------------|-------------|------------|-------------|------------|-------------|-----------|---------|----------| | 2^8 | 64 | 1.776 ms | 0.46% | 1.777 ms | 0.40% | 1.024 us | 0.06% | SAME | | 2^8 | 64 | 1.776 ms | 0.46% | 1.774 ms | 0.52% | -2.048 us | -0.12% | SAME | | 2^8 | 64 | 1.776 ms | 0.46% | 1.773 ms | 0.52% | -3.072 us | -0.17% | SAME | | 2^8 | 64 | 1.776 ms | 0.46% | 1.774 ms | 0.58% | -2.048 us | -0.12% | SAME | | 2^8 | 64 | 1.776 ms | 0.46% | 1.773 ms | 0.58% | -3.072 us | -0.17% | SAME | ``` and now it becomes ``` | BlockSize | NumBlocks | Ref Time | Ref Noise | Cmp Time | Cmp Noise | Diff | %Diff | Status | |-------------|-------------|------------|-------------|------------|-------------|-----------|---------|----------| | 2^8 | 64 | 1.776 ms | 0.46% | 1.777 ms | 0.40% | 1.024 us | 0.06% | SAME | | 2^8 | 64 | 1.773 ms | 0.64% | 1.774 ms | 0.52% | 1.024 us | 0.06% | SAME | | 2^8 | 64 | 1.774 ms | 0.46% | 1.773 ms | 0.52% | -1.024 us | -0.06% | SAME | | 2^8 | 64 | 1.773 ms | 0.46% | 1.774 ms | 0.58% | 1.024 us | 0.06% | SAME | | 2^8 | 64 | 1.774 ms | 0.52% | 1.773 ms | 0.58% | -1.024 us | -0.06% | SAME | ``` With the following raw data expected ``` (py313) opavlyk@NV-22T4X34:~/repos/nvbench$ jq '. | .benchmarks[] | .states[] | .summaries[] | select(.tag == "nv/cold/time/gpu/median") | .data[] | .value' base.json "0.0017756160497665405" "0.0017725440263748169" "0.001773568034172058" "0.0017725440263748169" "0.001773568034172058" (py313) opavlyk@NV-22T4X34:~/repos/nvbench$ jq '. | .benchmarks[] | .states[] | .summaries[] | select(.tag == "nv/cold/time/gpu/median") | .data[] | .value' test.json "0.0017766400575637818" "0.001773568034172058" "0.0017725440263748169" "0.001773568034172058" "0.0017725440263748169" ``` 2. nvbench_compare changes from using min_noise = min(ref_noise, cmp_noise) to using max_noise = max(ref_noise, cmp_noise) Using larger of ref and cmp noise level as a reference against which to gauge timing difference ratio makes more sense.
These measures are less sensitive to outliers
oleksandr-pavlyk
force-pushed
the
use-robust-centrality-dispersion-measures
branch
from
6e45072 to
25caa37
Compare
1 participant
This PR closes #342
std::array<ValueType, N> nvbench::detail::statistics::compute_percentiles(Iter begin, Iter end, std::array<int, N> percentiles)which finds requested percentile levels using "zero-based nearest endpoint-scaled rank".measure_cold."nv/cold/time/gpu/mean"and"nv/cold/time/gpu/stdev/relative"hidden, and replace them with"nv/cold/time/gpu/median"and"nv/cold/time/gpu/ir/relative", respectively."nv/cold/time/cpu*"stdrel_criterionto track"ir/relative"noise, i.e.nvbench_compareto reflect changes abovea. It now queries median for time value, and relative interquartile range as noise
b. Comparison logic has been changed to get reference noise from
min(ref_noise, cmp_noise)tomax(ref_noise, cmp_noise). The relative difference between durations should be permitted to be wider if one measurement is significantly noisier than another.Consequence of the change to
stdrel_criterionthe number of samples collected by default has become significantly smaller, especially with use of sufficient count of warm-up iterations.