Refactor perf bisect script _CalculateConfidence method.

tools/bisect-perf-regression.py

Index: tools/bisect-perf-regression.py
diff --git a/tools/bisect-perf-regression.py b/tools/bisect-perf-regression.py
index e15cabe1cde820eafaacf25b01994a166378511c..840c8f3ae426be1047a7e706922f6c8178b1bc9c 100755
--- a/tools/bisect-perf-regression.py
+++ b/tools/bisect-perf-regression.py
@@ -1375,7 +1375,7 @@ class BisectPerformanceMetrics(object):
         return True
       raise IOError('Missing extracted folder %s ' % output_dir)
     except e:
-      print 'Somewthing went wrong while extracting archive file: %s' % e
+      print 'Something went wrong while extracting archive file: %s' % e
       self.BackupOrRestoreOutputdirectory(restore=True)
       # Cleanup any leftovers from unzipping.
       if os.path.exists(output_dir):
@@ -2788,35 +2788,61 @@ class BisectPerformanceMetrics(object):
     return other_regressions
 
   def _CalculateConfidence(self, working_means, broken_means):
-    bounds_working = []
-    bounds_broken = []
-    for m in working_means:
-      current_mean = CalculateTruncatedMean(m, 0)
-      if bounds_working:
-        bounds_working[0] = min(current_mean, bounds_working[0])
-        bounds_working[1] = max(current_mean, bounds_working[0])
-      else:
-        bounds_working = [current_mean, current_mean]
-    for m in broken_means:
-      current_mean = CalculateTruncatedMean(m, 0)
-      if bounds_broken:
-        bounds_broken[0] = min(current_mean, bounds_broken[0])
-        bounds_broken[1] = max(current_mean, bounds_broken[0])
-      else:
-        bounds_broken = [current_mean, current_mean]
+    """Calculates the confidence percentage.
+
+    This is calculated based on how distinct the values are before and after
+    the last broken revision, and how noisy the results are.
+
+    Args:
+      working_means: A list of lists of "good" result numbers.

[qyearsley, 2014/03/25 02:25:33]

So, this argument name kind of confused me. Individual members of this list are
input to CalculateTruncatedMean, so that makes it seem like the members of this
list are lists of values, and are not means?

+      broken means: A list of lists of "bad" result numbers.
+
+    Returns:
+      A number between in the range [0, 100].
+    """
+    bounds_working = self._CalculateBounds(working_means)
+    bounds_broken = self._CalculateBounds(broken_means)
     dist_between_groups = min(math.fabs(bounds_broken[1] - bounds_working[0]),
-        math.fabs(bounds_broken[0] - bounds_working[1]))
+                              math.fabs(bounds_broken[0] - bounds_working[1]))
     working_mean = sum(working_means, [])
     broken_mean = sum(broken_means, [])
     len_working_group = CalculateStandardDeviation(working_mean)
     len_broken_group = CalculateStandardDeviation(broken_mean)
-
-    confidence = (dist_between_groups / (
-        max(0.0001, (len_broken_group + len_working_group ))))
+    confidence = (dist_between_groups /
+                  (max(0.0001, (len_broken_group + len_working_group))))
     confidence = int(min(1.0, max(confidence, 0.0)) * 100.0)
     return confidence
 
+  def _CalculateBounds(self, values_list):
+    """Returns the lower/upper bounds for the means of the given value lists.
+
+    Args:
+      values_list: A non-empty list of lists of numbers.
+
+    Returns:
+      A (lower, upper) pair of bounds.
+    """
+    bounds = None
+    for values in data:

[prasadv, 2014/03/25 16:43:47]

s/data/values_list

[qyearsley, 2014/03/25 17:31:19]

Done.

+      mean = CalculateTruncatedMean(values, 0)

[qyearsley, 2014/03/25 02:25:33]

The second argument to CalculateTruncatedMean is supposed to be a "The % from
the upper/lower portions of the data set to discards".

So wouldn't CalculateTruncatedMean(vals, 0) be just Mean(vals) ? Maybe it would
be clearer to add a function like the one below just to make it clear we're not
doing any fancy truncation stuff?

def Mean(x):
  return CalculateTruncatedMean(x, 0)

+      if bounds:
+        bounds[0] = min(current_mean, bounds[0])

[prasadv, 2014/03/25 16:43:47]

s/current_mean/mean

[qyearsley, 2014/03/25 17:31:19]

Done.

+        bounds[1] = max(current_mean, bounds[1])

[qyearsley, 2014/03/25 02:25:33]

Attention! This changes the behavior!

In the existing version on line 2797, max is calculated as:
  max(current_mean, bounds_working[0])

But it looks like bounds_working[0] is supposed to be the lower bound. Is it?
Does this seem right?

[shatch, 2014/03/25 19:56:21]

Nice catch! Yeah that should be max(current_mean, bounds_working[1]).

On 2014/03/25 02:25:33, qyearsley wrote:

[shatch, 2014/03/25 19:58:36]

BTW, if you want to experiment with changing this, go right ahead! It was just a
simple, conservative stab at calculating confidence, but it's pretty arbitrary.
Had plans to try out a t-test at some point to see if it gave better results.

On 2014/03/25 19:56:21, shatch wrote:

+      else:
+        bounds = (mean, mean)
+    return bounds
+
   def _GetResultsDict(self, revision_data, revision_data_sorted):
+    """Makes and returns a dictionary of overall results from the bisect.
+
+    Args:
+      revision_data: The revisions_data dict as returned by the Run method.
+      revision_data_sorted: A list of pairs from the above dictionary, sorted
+          in order of commits.
+
+    Returns:
+      A dictionary containing results from the bisect.
+    """
     # Find range where it possibly broke.
     first_working_revision = None
     first_working_revision_index = -1