[webkit-reviews] review granted: [Bug 188425] Show t-test results based on individual measurements to analysis task page : [Attachment 347900] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 22 22:51:55 PDT 2018


Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 188425: Show t-test results based on individual measurements to analysis
task page
https://bugs.webkit.org/show_bug.cgi?id=188425

Attachment 347900: Patch

https://bugs.webkit.org/attachment.cgi?id=347900&action=review




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 347900
  --> https://bugs.webkit.org/attachment.cgi?id=347900
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347900&action=review

> Websites/perf.webkit.org/public/shared/statistics.js:78
> +	       size += sample.iterationCount;

We should probably call this sampleSize.
It's important that this function doesn't randomly rely on the way measurement
set stores its data.
So it's better for the caller of sampleMeanAndVarianceFromMultipleSamples to be
manually converting
form one format to another.

> Websites/perf.webkit.org/public/shared/statistics.js:95
> +    this.probabilityRangeForWelchsTFromTwoSampleSets = function (sampleSet1,
sampleSet2) {

T-test is always constructed for two sets of samples so it's kind of redundant
to say "from two sample sets".
It's probably better to call this function
probabilityRangeForWelchsT*For*MultipleSamples to match
sampleMeanAndVarianceFromMultipleSamples.

> Websites/perf.webkit.org/public/shared/statistics.js:128
> +    this._combineWelchsT = function(stat1, stat2) {

This function is really the one that computes Welch's t.
We should probably call this _computeWelchsTFromStatistics.

> Websites/perf.webkit.org/public/shared/statistics.js:293
> +	   const binarySearchTable =
tDistributionTableForProbability.binarySearchTable;

Probably need a blank line here.
Also, we should probably add an early return where for when degreesOfFreedom is
greater than end of the last binary search table.

> Websites/perf.webkit.org/public/shared/statistics.js:297
> +	       let mid = low + parseInt((high - low) / 2);

Use Math.floor instead. Also use const.

> Websites/perf.webkit.org/public/shared/statistics.js:310
> +	       linearLookupTable: [

I don't think linearLookupTable is a good name here.
We should describe what the table is, not how it's used.
e.g. probabilityToTValue

> Websites/perf.webkit.org/public/shared/statistics.js:322
> +	       binarySearchTable: [

e.g. tValuesSortedByProbability

> Websites/perf.webkit.org/public/shared/statistics.js:323
> +		   {end: 101, value: 1.2900},  {end: 102, value: 1.2899}, 
{end: 103, value: 1.2898},  {end: 105, value: 1.2897},

Instead of {end: 101, value: 1.2900}, I'd call these {maxDF: 101, tValue:
1.2900} to make them more descriptive.

> Websites/perf.webkit.org/public/v3/models/test-group.js:172
> +	       const significanceLabelForMean = isSignificantForMean?
`significant with ${(probabilityRangeForMean.range[0] * 100).toFixed()}%
probability` : 'insignificant';

This should probably be extracted as a local lambda function to be used again.

> Websites/perf.webkit.org/public/v3/models/test-group.js:177
> +	       const probabilityRangeForIndividual =
Statistics.probabilityRangeForWelchsTFromTwoSampleSets(beforeMeasurements,
afterMeasurements);

Again, it's bad that Statistics.probabilityRangeForWelchsTFromTwoSampleSets is
implicitly relying on the property names used by MeasurementSet.

> Websites/perf.webkit.org/unit-tests/statistics-tests.js:257
> +	       return [firstHalf, secondHalf].map((values) => ({sum:
Statistics.sum(values), squareSum: Statistics.squareSum(values),
iterationCount: values.length}));

As I mentioned in person, we need test cases where this function returns an
array of length 1 & 3.


More information about the webkit-reviews mailing list