[Webkit-unassigned] [Bug 188425] Show t-test results based on individual measurements to analysis task page

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188425

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #347900|review?                     |review+
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180823/5a689c24/attachment.html>