[LNT] r237658 - Refactor ComparisonResult to be more self contained

James Molloy James.Molloy at arm.com
Thu Jun 4 00:35:09 PDT 2015


Hi Chris,

Yep, works for me!

Cheers,

James

On 3 Jun 2015, at 22:30, Chris Matthews <chris.matthews at apple.com<mailto:chris.matthews at apple.com>> wrote:

Hopefully fixed in r238974, or maybe now detectable by r238973 which checks we are being sane.  If it continues, can you grab me a �print locals()� inside the start of ComparisonResult.__init__.  I know the geomean calculation likes to pass Nones around and lists with None inside, but I don�t know what cases that is expected.  No tests in that part of the code.

I might refactor it a little more if I can get a handle on all the cases where Nones are getting passed through.

On Jun 3, 2015, at 7:20 AM, Chris Matthews <cmatthews5 at apple.com<mailto:cmatthews5 at apple.com>> wrote:

This looks familiar. I wonder if I fixed only locally. Will fix today!

Sent from my iPhone

On Jun 3, 2015, at 7:16 AM, James Molloy <james at jamesmolloy.co.uk<mailto:james at jamesmolloy.co.uk>> wrote:

Hi Chris,

I just updated our LNT instance and this commit causes the following internal server error:

... snip ...
  File "/home/llvm-test/sandbox/lnt/lnt/server/ui/templates/v4_run.html", line 334, in block "body"
    {% set cr = request_info.sri.get_geomean_comparison_result(
  File "/home/llvm-test/sandbox/lnt/lnt/server/reporting/analysis.py", line 286, in get_geomean_comparison_result
    bigger_is_better=field.bigger_is_better)
  File "/home/llvm-test/sandbox/lnt/lnt/server/reporting/analysis.py", line 60, in __init__
    self.stddev = stats.standard_deviation(samples)
  File "/home/llvm-test/sandbox/lnt/lnt/util/stats.py", line 36, in standard_deviation
    m = mean(l)
  File "/home/llvm-test/sandbox/lnt/lnt/util/stats.py", line 15, in mean
    return sum(l)/len(l)
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'

Could you please take a look?

Cheers,

James

[P.S: if you reply to james.molloy at arm.com<mailto:james.molloy at arm.com> perhaps I might get your reply - I still lay the blame at gmail's forwarding eating your messages]

On Tue, 19 May 2015 at 03:11 Chris Matthews <cmatthews5 at apple.com<mailto:cmatthews5 at apple.com>> wrote:
Author: cmatthews
Date: Mon May 18 20:59:20 2015
New Revision: 237658

URL: http://llvm.org/viewvc/llvm-project?rev=237658&view=rev<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D237658-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=dp8UqojU4x-vOe92vDtq0jOvYKDLqUFHYfOPEOw1V_0&e=>
Log:
Refactor ComparisonResult to be more self contained

Modified:
    lnt/trunk/lnt/server/reporting/analysis.py
    lnt/trunk/lnt/server/ui/templates/v4_run.html
    lnt/trunk/tests/server/reporting/analysis.py

Modified: lnt/trunk/lnt/server/reporting/analysis.py
URL: http://llvm.org/viewvc/llvm-project/lnt/trunk/lnt/server/reporting/analysis.py?rev=237658&r1=237657&r2=237658&view=diff<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lnt_trunk_lnt_server_reporting_analysis.py-3Frev-3D237658-26r1-3D237657-26r2-3D237658-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=a94GveESXxCe1huABIe9cbIWAHXj7YQp3fMDT79zatk&e=>
==============================================================================
--- lnt/trunk/lnt/server/reporting/analysis.py (original)
+++ lnt/trunk/lnt/server/reporting/analysis.py Mon May 18 20:59:20 2015
@@ -31,43 +31,67 @@ def calc_geomean(run_values):
 class ComparisonResult:
     """A ComparisonResult is ultimatly responsible for determining if a test
     improves, regresses or does not change, given some new and old data."""
-
-    def __init__(self,cur_value, prev_value, delta, pct_delta, stddev, MAD,
-                 cur_failed, prev_failed, samples, prev_samples, stddev_mean = None,
-                 confidence_lv = .05, bigger_is_better = False):
-        self.current = cur_value
-        self.previous = prev_value
-        self.delta = delta
-        self.pct_delta = pct_delta
-        self.stddev = stddev
-        self.MAD = MAD
+
+    def __init__(self, aggregation_fn,
+                 cur_failed, prev_failed, samples, prev_samples,
+                 confidence_lv=0.05, bigger_is_better=False):
+        self.aggregation_fn = aggregation_fn
+        if samples:
+            self.current = aggregation_fn(samples)
+        else:
+            self.current = None
+        if prev_samples:
+            self.previous = aggregation_fn(prev_samples)
+        else:
+            self.previous = None
+
+        # Compute the comparison status for the test value.
+        if self.current and self.previous and self.previous != 0:
+            self.delta = self.current - self.previous
+            self.pct_delta = self.delta / self.previous
+        else:
+            self.delta = 0
+            self.pct_delta = 0.0
+
+        # If we have multiple values for this run, use that to estimate the
+        # distribution.
+        if samples and len(samples) > 1:
+            self.stddev = stats.standard_deviation(samples)
+            self.MAD = stats.median_absolute_deviation(samples)
+        else:
+            self.stddev = None
+            self.MAD = None
+
+        self.stddev_mean = None  # Only calculate this if needed.
         self.failed = cur_failed
         self.prev_failed = prev_failed
         self.samples = samples
         self.prev_samples = prev_samples
-        self.stddev_mean = stddev_mean
+
         self.confidence_lv = confidence_lv
         self.bigger_is_better = bigger_is_better

+    @property
+    def stddev_mean(self):
+        """The mean around stddev for current sampples. Cached after first call.
+        """
+        if not self.stddev_mean:
+            self.stddev_mean = stats.mean(self.samples)
+        return self.stddev_mean
+
     def __repr__(self):
         """Print this ComparisonResult's constructor.
-
+
         Handy for generating test cases for comparisons doing odd things."""
-        frmt = "{}(" + "{}, " * 11 + ")"
-        return frmt.format("ComparisonResult",
-                           self.current,
-                           self.previous,
-                           self.delta,
-                           self.pct_delta,
-                           self.stddev,
-                           self.MAD,
-                           self.failed,
-                           self.prev_failed,
-                           self.samples,
-                           self.prev_samples,
-                           self.stddev_mean,
-                           self.confidence_lv,
-                           self.bigger_is_better)
+        fmt = "{}(" + "{}, " * 7 + ")"
+        return fmt.format(self.__class__.__name__,
+                          self.aggregation_fn.__name__,
+                          self.failed,
+                          self.prev_failed,
+                          self.samples,
+                          self.prev_samples,
+                          self.confidence_lv,
+                          bool(self.bigger_is_better))

     def is_result_interesting(self):
         """is_result_interesting() -> bool
@@ -237,77 +261,27 @@ class RunInfo(object):
                       if s[field.index] is not None]
         prev_values = [s[field.index] for s in prev_samples
                        if s[field.index] is not None]
-        if run_values:
-            run_value = self.aggregation_fn(run_values)
-        else:
-            run_value = None
-        if prev_values:
-            prev_value = self.aggregation_fn(prev_values)
-        else:
-            prev_value = None
-
-        # If we have multiple values for this run, use that to estimate the
-        # distribution.
-        if run_values and len(run_values) > 1:
-            stddev = stats.standard_deviation(run_values)
-            MAD = stats.median_absolute_deviation(run_values)
-            stddev_mean = stats.mean(run_values)
-        else:
-            stddev = None
-            MAD = None
-            stddev_mean = None
-
-        # If we are missing current or comparison values we are done.
-        if run_value is None or prev_value is None:
-            return ComparisonResult(
-                run_value, prev_value, delta=None,
-                pct_delta = None, stddev = stddev, MAD = MAD,
-                cur_failed = run_failed, prev_failed = prev_failed,
-                samples = run_values, prev_samples = prev_values,
-                confidence_lv = self.confidence_lv,
-                bigger_is_better = field.bigger_is_better)
-
-        # Compute the comparison status for the test value.
-        delta = run_value - prev_value
-        if prev_value != 0:
-            pct_delta = delta / prev_value
-        else:
-            pct_delta = 0.0
-
-        return ComparisonResult(run_value, prev_value, delta,
-                                pct_delta, stddev, MAD,
-                                run_failed, prev_failed, run_values,
-                                prev_values, stddev_mean, self.confidence_lv,
-                                bigger_is_better = field.bigger_is_better)
-
+
+        r = ComparisonResult(self.aggregation_fn,
+                             run_failed, prev_failed, run_values,
+                             prev_values, self.confidence_lv,
+                             bigger_is_better=field.bigger_is_better)
+        print repr(r)
+        return r

-    def get_geomean_comparison_result(self, run, compare_to, field, tests,
-                                      comparison_window=[]):
+    def get_geomean_comparison_result(self, run, compare_to, field, tests):
         if tests:
             prev_values,run_values = zip(*[(cr.previous,cr.current) for _,_,cr in tests])
         else:
             prev_values,run_values = [], []

-        run_geomean = calc_geomean(run_values)
-        prev_geomean = calc_geomean(prev_values)
-
-        if run_geomean and prev_geomean:
-            delta = run_geomean - prev_geomean
-            if prev_geomean != 0:
-                pct_delta = delta / prev_geomean
-            else:
-                pct_delta = 0.0
-        else:
-            delta = pct_delta = 0
-
-        return ComparisonResult(run_geomean, prev_geomean, delta,
-                                pct_delta, stddev=None, MAD=None,
-                                cur_failed=run_values and not run_geomean,
-                                prev_failed=prev_values and not prev_geomean,
-                                samples=[run_geomean] if run_geomean else [],
-                                prev_samples=[prev_geomean] if prev_geomean else [],
+        return ComparisonResult(calc_geomean,
+                                cur_failed=bool(run_values),
+                                prev_failed=bool(prev_values),
+                                samples=run_values,
+                                prev_samples=prev_values,
                                 confidence_lv=0,
-                                bigger_is_better = field.bigger_is_better)
+                                bigger_is_better=field.bigger_is_better)

     def _load_samples_for_runs(self, run_ids):
         # Find the set of new runs to load.

Modified: lnt/trunk/lnt/server/ui/templates/v4_run.html
URL: http://llvm.org/viewvc/llvm-project/lnt/trunk/lnt/server/ui/templates/v4_run.html?rev=237658&r1=237657&r2=237658&view=diff<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lnt_trunk_lnt_server_ui_templates_v4-5Frun.html-3Frev-3D237658-26r1-3D237657-26r2-3D237658-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=QXpbzkMZse99YZyDX5HVN2Zx9PkBRLGpeRbNT6msGpo&e=>
==============================================================================
--- lnt/trunk/lnt/server/ui/templates/v4_run.html (original)
+++ lnt/trunk/lnt/server/ui/templates/v4_run.html Mon May 18 20:59:20 2015
@@ -332,7 +332,7 @@
         </tbody>
         <tfoot>
           {% set cr = request_info.sri.get_geomean_comparison_result(
-                      run, compare_to, field, tests, request_info.comparison_window) %}
+                      run, compare_to, field, tests) %}
           <td><input type="checkbox" name="mean" value="{{machine.id<https://urldefense.proofpoint.com/v2/url?u=http-3A__machine.id&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=gAmmnPAoTUCrKVDFvfJ8x_Aa2oJr3HSn_xkYyghkybw&e=>}}.{{field.index}}"></td>
           <td><a href="{{graph_base}}&mean={{machine.id<https://urldefense.proofpoint.com/v2/url?u=http-3A__machine.id&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=gAmmnPAoTUCrKVDFvfJ8x_Aa2oJr3HSn_xkYyghkybw&e=>}}.{{field.index}}">Geometric Mean</a></td>
           {{ get_cell_value(cr) }}

Modified: lnt/trunk/tests/server/reporting/analysis.py
URL: http://llvm.org/viewvc/llvm-project/lnt/trunk/tests/server/reporting/analysis.py?rev=237658&r1=237657&r2=237658&view=diff<https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lnt_trunk_tests_server_reporting_analysis.py-3Frev-3D237658-26r1-3D237657-26r2-3D237658-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=X3KNE8s_qHD2EVtyXDGw3v6aMKY0Ju48Rkw-hpHTjqI&e=>
==============================================================================
--- lnt/trunk/tests/server/reporting/analysis.py (original)
+++ lnt/trunk/tests/server/reporting/analysis.py Mon May 18 20:59:20 2015
@@ -4,7 +4,7 @@
 import unittest
 import lnt.util.stats as stats
 from lnt.server.reporting.analysis import ComparisonResult, REGRESSED, IMPROVED
-from lnt.server.reporting.analysis import UNCHANGED_PASS
+from lnt.server.reporting.analysis import UNCHANGED_PASS, UNCHANGED_FAIL


 class ComparisonResultTest(unittest.TestCase):
@@ -13,15 +13,8 @@ class ComparisonResultTest(unittest.Test
     def test_comp(self):
         """Test a real example."""
         curr_samples = [0.0887, 0.0919, 0.0903]
-        prev = 0.0858
-        cur = min(curr_samples)
-        stddev = stats.standard_deviation(curr_samples)
-        MAD = stats.median_absolute_deviation(curr_samples)
-        stddev_mean = stats.mean(curr_samples)
-        uninteresting = ComparisonResult(cur, prev, cur-prev,
-                                         (cur-prev)/prev, stddev, MAD,
-                                         False, False, curr_samples, [prev],
-                                         stddev_mean)
+        prev = [0.0858]
+        uninteresting = ComparisonResult(min, False, False, curr_samples, prev)

         self.assertFalse(uninteresting.is<https://urldefense.proofpoint.com/v2/url?u=http-3A__uninteresting.is&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=LNXkZ03tcVIIBrsTu6UL5jOhzxYt1FE16ln5zGl35Es&e=>_result_interesting())
         self.assertEquals(uninteresting.get_test_status(), UNCHANGED_PASS)
@@ -29,31 +22,36 @@ class ComparisonResultTest(unittest.Test

     def test_slower(self):
         """Test getting a simple regression."""
-        slower = ComparisonResult(10, 5, 5, 0.5, None, None,
-                                  False, False, [10], [5], None)
+        slower = ComparisonResult(min,
+                                  False, False, [10], [5])
         self.assertEquals(slower.get_value_status(), REGRESSED)
         self.assertTrue(slower.is<https://urldefense.proofpoint.com/v2/url?u=http-3A__slower.is&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=Z5kxq32Ae5b15BStaNJTr9T0hhnF6FXiedX6JhP_eq8&e=>_result_interesting())

     def test_faster(self):
         """Test getting a simple improvement."""

-        faster = ComparisonResult(5, 10, -5, -0.5, None, None,
-                                  False, False, [5], [10], None)
+        faster = ComparisonResult(min,
+                                  False, False, [5], [10])
         self.assertEquals(faster.get_value_status(), IMPROVED)
         self.assertTrue(faster.is<https://urldefense.proofpoint.com/v2/url?u=http-3A__faster.is&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hlSrqTcmRBON9OCba2v2Zn0fIsNvy5cwSdLlBAkG2wo&s=T7kqvyItOJV5W2QAHss-VlNdSKjL4YpLZomO4eECkD0&e=>_result_interesting())

     def test_improved_status(self):
         """Test getting a test status improvement."""
-        improved = ComparisonResult(None, None, None, None, None, None,
-                                    False, True, [5], [10], None)
+        improved = ComparisonResult(min,
+                                    False, True, [1], None)
         self.assertEquals(improved.get_test_status(), IMPROVED)

     def test_regressed_status(self):
         """Test getting a test status improvement."""
-        improved = ComparisonResult(None, None, None, None, None, None,
-                                    True, False, [5], [10], None)
+        improved = ComparisonResult(min,
+                                    True, False, None, [10])
         self.assertEquals(improved.get_test_status(), REGRESSED)

+    def test_keep_on_failing_status(self):
+        """Test getting a repeated fail."""
+        improved = ComparisonResult(min,
+                                    True, True, None, None)
+        self.assertEquals(improved.get_test_status(), UNCHANGED_FAIL)

 if __name__ == '__main__':
     unittest.main()


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits



-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150604/e2d6e57f/attachment.html>


More information about the llvm-commits mailing list