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

Fixed plotting bug when all samples' measurements have the same value #832

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

vxfield
Copy link

@vxfield vxfield commented Dec 10, 2024

This PR fixes #720.

Overview of the bug

Criterion panics when generating PDF plots with gnuplot or plotters and all measurements in a sample have the same duration.

Root cause

  1. The root cause is that the KDE bandwidth estimation returns zero (because the standard deviation is zero) when all measurements in a sample have the same value
  2. That causes a division by zero error (NaN) in the KDE's probability density estimation ((... / h) when h==0)
  3. That causes an assertion panic assertion failed: slice.len() > 1 && slice.iter().all(|x| !x.is_nan()) when initializing a Sample.

When does it happen?

There are mainly 3 scenarios in which all samples have the same duration:

  1. When Criterion aborts extremely long running benchmarks and create a sample with two "identical values", in which existing hack to prevent gnuplot bug may fail criterion panicking with slice.len() > 1 && slice.iter().all(|x| !x.is_nan()) #720.
  2. When the developer returns the same duration per iteration in a iter_custom benchmark (for example, when the having a custom logic that sets a minimum value for the duration to be, useful when comparing O(1) and O(n) functions).
  3. By pure, and almost impossible, chance when multiple measurements are the same at the nanosecond level.

Changes

This PR fixes the root cause of the bug and slightly improves the plotting of PDF charts when all measurements in a sample have the same value.

  1. First we increase the test coverage to reproduce the error: d94aad7 and cf44f55
  2. Then we add a test case for the bandwidth estimation 21d5353
  3. We fix the bug in the bandwidth estimation ca6f1bc
  4. Then we add a test case for the KDE estimation e160bed
  5. We fix the bug in the KDE estimation 0937444
  6. We also fix the error Gnuplot:Can't plot with an empty x range! 7cf32eb
  7. And finally we improve the plot of PDF charts for this scenario b2279f3
    Before:
    image
    After:
    image

@vxfield vxfield marked this pull request as ready for review December 10, 2024 00:45
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.

criterion panicking with slice.len() > 1 && slice.iter().all(|x| !x.is_nan())
1 participant