Summary: | Summary page should support calculating summary using weighted mean. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | dewei_zhu | ||||||
Component: | New Bugs | Assignee: | dewei_zhu | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, dewei_zhu, rniwa | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
dewei_zhu
2021-09-26 17:49:05 PDT
Created attachment 439301 [details]
Patch
Comment on attachment 439301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439301&action=review > Websites/perf.webkit.org/public/shared/statistics.js:19 > + this.weightedMean = function (values) { It's confusing to call this argument "values". Maybe valuesWithWeights? > Websites/perf.webkit.org/public/shared/statistics.js:27 > + if (!Array.isArray(item)) { > + totalWeight += 1; > + sum += item; > + continue; > + } I don't think we should do polymorphism like this. > Websites/perf.webkit.org/public/shared/statistics.js:31 > + const [value, weight] = item; > + totalWeight += weight; > + sum += value * weight; Why not just use a dictionary / object? e.g. totalWeight += item.weight; sum += item.weight * item.value; > Websites/perf.webkit.org/public/v3/pages/summary-page.js:17 > + this._customWeightConfigurations = summarySettings.customWeightConfigurations; It doesn't seem useful to say this is custom weight? How about just weightedConfigurations? > Websites/perf.webkit.org/public/v3/pages/summary-page.js:352 > + if (typeof weightForPlatform === 'number') There is no point in using === here. The value of typeof is always a string. But it seems problematic to assume anything that's not a number is an object? e.g. string and null. Created attachment 439318 [details]
Patch
Comment on attachment 439318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439318&action=review We really need tests for summary page... > Websites/perf.webkit.org/public/v3/pages/summary-page.js:17 > + this._weightConfigurations = summarySettings.weightConfigurations; This sounds like the configuration of weights. So I think it should be weightedConfigurations. Like excluded configurations, we're listing a list of configurations whose weight isn't 1 along with its weight. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:354 > + const isNumber = (object) => ['number', 'string'].includes(typeof object) && !isNaN(+object); > + if (isNumber(weightForPlatform)) > + return +weightForPlatform; This isn't quite right. This will throw an exception if weightForPlatform is null or true/false as well. I think what we want to check instead is simply isNaN(+object). i.e. if (!isNaN(weightForPlatform)) return weightForPlatform; > Websites/perf.webkit.org/public/v3/pages/summary-page.js:358 > + const weight = weightForPlatform[metricId] > + if (isNumber(weight)) > + return +weight; Here, we probably want to do instead: if (typeof weightForPlatform != 'object' || isNaN(+weightForPlatform[metricId])) return 1; |