Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309143006:
[Telemetry] Update the ConfindenceInterval calculation in results.html to use (Closed)
Description[Telemetry] Update the ConfindenceInterval calculation in results.html to use
provided std from value.
BUG=516344
Committed: https://crrev.com/132e63c60acba0926c383929dd9050e1285f968d
Cr-Commit-Position: refs/heads/master@{#348172}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Address review comments #
Total comments: 6
Patch Set 3 : Address petrcermak's comment #
Total comments: 2
Messages
Total messages: 24 (6 generated)
Patchset #1 (id:1) has been deleted
nednguyen@google.com changed reviewers: + eakuefner@chromium.org, petrcermak@chromium.org
I am cringe at the lack of tests to the results.html code but adding infra for testing it would be a sizable project and should be done after telemetry is moved to catapult. +Petrcermak: please run this on local and let me know whether the results are expected.
On 2015/09/09 at 18:24:08, nednguyen wrote: > I am cringe at the lack of tests to the results.html code but adding infra for testing it would be a sizable project and should be done after telemetry is moved to catapult. > > +Petrcermak: please run this on local and let me know whether the results are expected. lgtm but please add "modified statistics computation code in PerformanceTests/resources/statistics.js" to the local modifications section of WebKit's README.chromium. We should eventually move away from Telemetry depending on Blink/WebKit, but I think that goes along with rewriting HTMLOutputFormatter.
Here's a couple of comments. Thanks, Petr https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, nit: This should probably be indented +4 spaces https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1368: run, rawMetrics[metricName].std)); nit: This should probably be indented 4 spaces (+2) https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/internal/results/html_output_formatter_unittest.py (left): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/internal/results/html_output_formatter_unittest.py:103: self.assertEquals(expected, formatter.GetResults()) Should this really be removed?? https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js (right): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:79: var degreesOfFreedom = numberOfSamples - 1; The degrees of freedom should be changed as well. For example if get 3 samples on 2 pages, there will be only 2*(3-1)=4 degrees of freedom (not 2*3-1=5) when using the new calculation (because we have 2 means). https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:93: this.confidenceIntervalDelta = function (confidenceLevel, numberOfSamples, sum, squareSum) { To reduce code duplication, this function should use confidenceIntervalDeltaFromStd: this.confidenceIntervalDelta = function (confidenceLevel, numberOfSamples, sum, squareSum) { var sampleStandardDeviation = this.sampleStandardDeviation(numberOfSamples, sum, squareSum); return this.confidenceIntervalDeltaFromStd(confidenceLevel, numberOfSamples, sampleStandardDeviation); }
BTW, the title of the patch is incomplete (due to a line break in the description). Petr
https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, On 2015/09/09 18:44:40, petrcermak wrote: > nit: This should probably be indented +4 spaces Done. https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:1368: run, rawMetrics[metricName].std)); On 2015/09/09 18:44:40, petrcermak wrote: > nit: This should probably be indented 4 spaces (+2) Done. https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js (right): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:79: var degreesOfFreedom = numberOfSamples - 1; On 2015/09/09 18:44:40, petrcermak wrote: > The degrees of freedom should be changed as well. For example if get 3 samples > on 2 pages, there will be only 2*(3-1)=4 degrees of freedom (not 2*3-1=5) when > using the new calculation (because we have 2 means). Done. But this is no-op change for now. I will hook up degree of freedom change to list_of_scalar_value in the next patch. https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:93: this.confidenceIntervalDelta = function (confidenceLevel, numberOfSamples, sum, squareSum) { On 2015/09/09 18:44:40, petrcermak wrote: > To reduce code duplication, this function should use > confidenceIntervalDeltaFromStd: > > this.confidenceIntervalDelta = function (confidenceLevel, numberOfSamples, sum, > squareSum) { > var sampleStandardDeviation = this.sampleStandardDeviation(numberOfSamples, > sum, squareSum); > return this.confidenceIntervalDeltaFromStd(confidenceLevel, numberOfSamples, > sampleStandardDeviation); > } Done.
LGTM with 3 more comments. Thanks, Petr https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js (right): https://codereview.chromium.org/1309143006/diff/20001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:79: var degreesOfFreedom = numberOfSamples - 1; On 2015/09/09 22:29:06, nednguyen wrote: > On 2015/09/09 18:44:40, petrcermak wrote: > > The degrees of freedom should be changed as well. For example if get 3 samples > > on 2 pages, there will be only 2*(3-1)=4 degrees of freedom (not 2*3-1=5) when > > using the new calculation (because we have 2 means). > > Done. But this is no-op change for now. I will hook up degree of freedom change > to list_of_scalar_value in the next patch. Great, thanks! https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:282: function TestResult(metric, values, associatedRun, std, degreeOfFreedom) { nit: s/degreeOfFreedom/degreesOfFreedom/g (should be plural, applies to all files in this patch) https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:79: var degreesOfFreedom = opt_degreeOfFreedom || numberOfSamples - 1; In theory (and most probably in practice as well), opt_degreeOfFreedom could be zero (e.g. if you did only a single measurement per page on K pages, the degrees of freedom would be K*(1 - 1) = 0. In that case, degreesOfFreedom would incorrectly end up being K - 1. I think you should do something along the lines of: var degreesOfFreedom = opt_degreeOfFreedom; if (degreesOfFreedom === undefined) degreesOfFreedom = numberOfSamples - 1; Alternatively, you could use the ternary operator: var degreesOfFreedom = opt_degreeOfFreedom === udnefined ? numberOfSamples - 1 : opt_degreeOfFreedom; https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/README.chromium (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/README.chromium:13: given std & degree of freedom (see https://codereview.chromium.org/1309143006) nit: s/degree/degrees/ It would probably also be a little more readable to do s/std/standard deviation/ And maybe s/given/custom/ :-)?
https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:282: function TestResult(metric, values, associatedRun, std, degreeOfFreedom) { On 2015/09/10 09:04:09, petrcermak wrote: > nit: s/degreeOfFreedom/degreesOfFreedom/g (should be plural, applies to all > files in this patch) Done. https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js:79: var degreesOfFreedom = opt_degreeOfFreedom || numberOfSamples - 1; On 2015/09/10 09:04:09, petrcermak wrote: > In theory (and most probably in practice as well), opt_degreeOfFreedom could be > zero (e.g. if you did only a single measurement per page on K pages, the degrees > of freedom would be K*(1 - 1) = 0. In that case, degreesOfFreedom would > incorrectly end up being K - 1. I think you should do something along the lines > of: > > var degreesOfFreedom = opt_degreeOfFreedom; > if (degreesOfFreedom === undefined) > degreesOfFreedom = numberOfSamples - 1; > > Alternatively, you could use the ternary operator: > > var degreesOfFreedom = opt_degreeOfFreedom === udnefined ? numberOfSamples - 1 : > opt_degreeOfFreedom; Done. I picked the first one. https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... File tools/telemetry/third_party/WebKit/README.chromium (right): https://codereview.chromium.org/1309143006/diff/40001/tools/telemetry/third_p... tools/telemetry/third_party/WebKit/README.chromium:13: given std & degree of freedom (see https://codereview.chromium.org/1309143006) On 2015/09/10 09:04:09, petrcermak wrote: > nit: s/degree/degrees/ > > It would probably also be a little more readable to do s/std/standard deviation/ > > And maybe s/given/custom/ :-)? Done.
The CQ bit was checked by nednguyen@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1309143006/#ps60001 (title: "Address petrcermak's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309143006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309143006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309143006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309143006/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/132e63c60acba0926c383929dd9050e1285f968d Cr-Commit-Position: refs/heads/master@{#348172}
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/132e63c60acba0926c383929dd9050e1285f968d Cr-Commit-Position: refs/heads/master@{#348172}
Message was sent while issue was closed.
https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, On my environment, I consistently see the following error, preventing the results from getting shown: > Uncaught TypeError: Statistics.confidenceIntervalDeltaFromStd is not a function Seems like confidenceIntervalDeltaFromStd is not defined. At least there's none in third_party/WebKit/PerformanceTests/resources/statistics.js. Can you fix this?
Message was sent while issue was closed.
https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... File tools/telemetry/support/html_output/results-template.html (right): https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, On 2015/09/28 04:32:37, Yuta Kitamura wrote: > On my environment, I consistently see the following error, > preventing the results from getting shown: > > > Uncaught TypeError: Statistics.confidenceIntervalDeltaFromStd is not a > function > > Seems like confidenceIntervalDeltaFromStd is not defined. > At least there's none in > third_party/WebKit/PerformanceTests/resources/statistics.js. > > Can you fix this? I cannot reproduce this error (see http://imgur.com/A6DYJBL). This is really strange, can you check src/tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js which is the one telemetry uses?
Message was sent while issue was closed.
On 2015/09/28 at 15:33:23, nednguyen wrote: > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > File tools/telemetry/support/html_output/results-template.html (right): > > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, > On 2015/09/28 04:32:37, Yuta Kitamura wrote: > > On my environment, I consistently see the following error, > > preventing the results from getting shown: > > > > > Uncaught TypeError: Statistics.confidenceIntervalDeltaFromStd is not a > > function > > > > Seems like confidenceIntervalDeltaFromStd is not defined. > > At least there's none in > > third_party/WebKit/PerformanceTests/resources/statistics.js. > > > > Can you fix this? > > I cannot reproduce this error (see http://imgur.com/A6DYJBL). This is really strange, can you check src/tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js which is the one telemetry uses? There's nothing containing "FromStd": https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Pe...
Message was sent while issue was closed.
On 2015/09/29 at 03:47:14, Yuta Kitamura wrote: > On 2015/09/28 at 15:33:23, nednguyen wrote: > > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > > File tools/telemetry/support/html_output/results-template.html (right): > > > > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > > tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, > > On 2015/09/28 04:32:37, Yuta Kitamura wrote: > > > On my environment, I consistently see the following error, > > > preventing the results from getting shown: > > > > > > > Uncaught TypeError: Statistics.confidenceIntervalDeltaFromStd is not a > > > function > > > > > > Seems like confidenceIntervalDeltaFromStd is not defined. > > > At least there's none in > > > third_party/WebKit/PerformanceTests/resources/statistics.js. > > > > > > Can you fix this? > > > > I cannot reproduce this error (see http://imgur.com/A6DYJBL). This is really strange, can you check src/tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js which is the one telemetry uses? > > There's nothing containing "FromStd": > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Pe... Ah, under tools/telemetry/! I didn't know telemetry has its own copy of statistics.js... Sorry for the noise. (A bit more context: I have my own tool to rebuild results.html and it pulls statistics.js from a real Blink directory.)
Message was sent while issue was closed.
On 2015/09/29 at 03:50:54, yutak wrote: > On 2015/09/29 at 03:47:14, Yuta Kitamura wrote: > > On 2015/09/28 at 15:33:23, nednguyen wrote: > > > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > > > File tools/telemetry/support/html_output/results-template.html (right): > > > > > > https://codereview.chromium.org/1309143006/diff/60001/tools/telemetry/support... > > > tools/telemetry/support/html_output/results-template.html:325: return metric.scalingFactor() * Statistics.confidenceIntervalDeltaFromStd(0.95, values.length, > > > On 2015/09/28 04:32:37, Yuta Kitamura wrote: > > > > On my environment, I consistently see the following error, > > > > preventing the results from getting shown: > > > > > > > > > Uncaught TypeError: Statistics.confidenceIntervalDeltaFromStd is not a > > > > function > > > > > > > > Seems like confidenceIntervalDeltaFromStd is not defined. > > > > At least there's none in > > > > third_party/WebKit/PerformanceTests/resources/statistics.js. > > > > > > > > Can you fix this? > > > > > > I cannot reproduce this error (see http://imgur.com/A6DYJBL). This is really strange, can you check src/tools/telemetry/third_party/WebKit/PerformanceTests/resources/statistics.js which is the one telemetry uses? > > > > There's nothing containing "FromStd": > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Pe... > > Ah, under tools/telemetry/! I didn't know telemetry has its own copy of statistics.js... > > Sorry for the noise. > > (A bit more context: I have my own tool to rebuild results.html and it pulls statistics.js > from a real Blink directory.) You could probably upstream Telemetry's local modifications to statistics.js if that would be helpful. We've just been using it as a repository for JS statistics helpers, so there haven't been any plumbing changes that would break compatibility with PerformanceTests. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
