Repository navigation
complexity: filter skipped runs and validate sample count in ComputeBigO - #2330
jdymitarai wants to merge 1 commit into
Conversation
|
|
||
| } // end namespace | ||
|
|
||
| std::vector<BenchmarkReporter::Run> ComputeBigO( |
There was a problem hiding this comment.
Changes to this function look good.
There was a problem hiding this comment.
Forward progress suggestion:
I'd propose to keep only the changes to this function in this PR,
and deal with the rest in another PR.
(I'm not yet convinced about those other changes.)
| // Calculate complexity. | ||
| result.coef = sigma_time_gn / sigma_gn_squared; | ||
| // Calculate complexity. Guard against division by zero if all fitting points evaluate to zero. | ||
| result.coef = (std::fpclassify(sigma_gn_squared) == FP_ZERO || sigma_gn_squared <= 0.0) |
| result.rms = (std::fpclassify(mean) == FP_ZERO || | ||
| std::fpclassify(sigma_gn_squared) == FP_ZERO) |
There was a problem hiding this comment.
Same, why not just <= ?
| for (const auto& fit : fit_curves) { | ||
| LeastSq current_fit = MinimalLeastSq(n, time, FittingCurve(fit)); | ||
| if (current_fit.rms < best_fit.rms) { | ||
| if (!std::isnan(current_fit.rms) && current_fit.rms < best_fit.rms) { |
There was a problem hiding this comment.
But can we end up with a NAN here after all?
| return n <= 0 ? 0.0 | ||
| : static_cast<double>(n) * | ||
| std::log2(static_cast<double>(n)); | ||
| }; |
There was a problem hiding this comment.
I do not understand. How n can be 0 here?
6971dcc to
12dbc7e
Compare
jdymitarai
changed the title
|
Thanks for the review and the suggestion! I have updated the branch to isolate the changes strictly to ComputeBigO and its unit test for filtering skipped runs, reverting the unrelated adjustments to FittingCurve and MinimalLeastSq. Ready for another look. |
|
@googlebot I signed it! |
12dbc7e to
855b389
Compare
| 0.32888184094918121, 1e-15); | ||
| } | ||
|
|
||
| TEST(ComplexityTest, ComputeBigOFiltersSkippedRuns) { |
There was a problem hiding this comment.
Why does this need to be a gtest and not a usual output test?
This branch has not been deployed
2 participants
When benchmark runs within a family are skipped via SkipWithError or SkipWithMessage, their iterations count evaluates to zero, causing ComputeBigO to divide accumulated time by zero and produce NaN values across BigO and RMS outputs. Furthermore, unlike ComputeStats which cleanly filters out non-successful runs, ComputeBigO previously processed all report entries unconditionally and accessed reports[0] for metadata even if the first run had failed or been skipped. Additionally, when all complexity points evaluate to zero under least-squares fitting (such as log2(1) under oLogN) or when non-positive sizes are passed, the fitting denominator vanishes and yields invalid floating-point values.
This change aligns ComputeBigO with the successful-run filtering pattern established in ComputeStats, requiring at least two valid runs with non-zero iterations before calculating BigO aggregates and utilizing the first successful run for reporter metadata. In FittingCurve and MinimalLeastSq, non-positive complexity sizes for logarithmic curves and zero-valued least-squares denominators are defensively guarded to return zero coefficients rather than generating division-by-zero anomalies or propagating NaNs. Unit tests in statistics_gtest verify that skipped runs are ignored and zero-denominator regressions are properly handled.