Added UI to show potential regressions in chart with t-testing against segmentations.
Created attachment 336761 [details] Patch
Created attachment 336807 [details] Patch
Comment on attachment 336807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336807&action=review > Websites/perf.webkit.org/public/shared/statistics.js:113 > + this.tTestForRangeWithSignificance = function (values, segmentations, significance) { This is not running t-test. We're finding ranges which reject null hypothesis based on segmentations. How about findRangesForChangeDetectionsWithWelchsTTest? > Websites/perf.webkit.org/public/shared/statistics.js:124 > + const currentTestingIndex = segmentations[i].seriesIndex; What does testing index mean? I think this is really a change point: https://en.wikipedia.org/wiki/Change_detection So it should be named as such: currentChangePoint > Websites/perf.webkit.org/public/shared/statistics.js:126 > + const leftBound = segmentations[i - 1].seriesIndex; > + const rightBound = segmentations[i + 2].seriesIndex; Use start/end instead of left/right. "Bound" is ambiguous still. This should be called previousChangePoint and nextChangePoint respectively. Do we always include 0 and length - 1 in the segmentations? If not, we'd not detect a change if the graph had exactly one change point. > Websites/perf.webkit.org/public/shared/statistics.js:128 > + for (let leftEdge = currentTestingIndex - 2, rightEdge = currentTestingIndex + 2; leftEdge >= leftBound && rightEdge <= rightBound; leftEdge--, rightEdge++) { Why are we always adding & subtracting two? > Websites/perf.webkit.org/public/shared/statistics.js:129 > + const result = this.computeWelchsT(values, leftEdge, currentTestingIndex - leftEdge, values, currentTestingIndex, rightEdge - currentTestingIndex, 2 * significance - 1); What does 2 * significance - 1 mean here? Are we converting one-sided probability to two-sided probability or something? If so, we should store that in a local variable with a descriptive name. > Websites/perf.webkit.org/public/v3/components/chart-pane-base.js:176 > + if(annotation.task) Nit: Missing a space between if and (. > Websites/perf.webkit.org/public/v3/components/time-series-chart.js:159 > + setSuggestedAnnotations(suggestedAnnotations) > + { > + this._suggestedAnnoations = suggestedAnnotations; > + this._annotationRows = null; > + this.enqueueToRender(); > + } I don't think it makes sense for TimeSeriesChart to provide a facility to merge the list of annotations like this. ChartPane should simply combine the list of analysis tasks & annotations for change point detections in a single array instead. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:50 > + function valueForIndexInSegmentation(index) { Where is this function ever used? > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:68 > + fillStyle: '#808080', Put this in ChartStyles as we talked about it in person.
Created attachment 336999 [details] Patch
Comment on attachment 336999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336999&action=review > Websites/perf.webkit.org/ChangeLog:17 > + (Statistics.new.this.findRangesForChangeDetectionsWithWelchsTTest): Add a description as to why we're incrementing / decrementing the index by 2. > Websites/perf.webkit.org/public/v3/components/chart-pane-base.js:265 > + this._renderAnnotationsLazily.evaluate(this._tasksForAnnotations, this._detectedAnnotations, this._renderedAnnotations); Just get the list of change types for each task, and add that to the argument as in: _renderAnnotationsLazily.evaluate(this._tasksForAnnotations, this._detectedAnnotations, ...this._tasksForAnnotations.map((task) => task.changeType())); Alternatively, just re-construct ._tasksForAnnotations or _detectedAnnotations whenever we're currently setting _renderedAnnotations = false. > Websites/perf.webkit.org/public/v3/components/chart-pane-base.js:-299 > - fillStyle: fillStyle, We should be computing the fill style here instead. > Websites/perf.webkit.org/public/v3/components/time-series-chart.js:408 > - context.fillStyle = bar.fillStyle; > + context.fillStyle = this._options.annotations.fillStyleForTask(bar.task); We need to pre-determine the style. TimeSeriesChart shouldn't be aware of a task. > Websites/perf.webkit.org/public/v3/models/metric.js:88 > + summarizeForValues(beforeValue, afterValue, betterDescription, worseDescription) This is a very vague/generic name. How about labelForDifference? Also, description is usually used to refer a long paragraph of text. Why not progressionTypeName and regressionTypeName. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:43 > + label: 'Segmentation with t-test analysis', Again, we should call this "Segmentation with Welch's t-test change detection" > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:52 > + segmentation.analysisAnnotations = Statistics.findRangesForChangeDetectionsWithWelchsTTest(timeSeries.values(), segmentation, parameters[parameters.length - 1]).map((range) => { Please wrap the line earlier or store the arguments in a local variable so that this code is easier to read. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:68 > + {label: "t-test significance", value: 0.99, type: 'select', options: Statistics.supportedOneSideTTestProbabilities()}, We don't need to have both "options" and "type". Just detect the presence of "options". > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:450 > + if (parameter.type == 'select') { Just do: "options" in parameter. > Websites/perf.webkit.org/public/v3/pages/chart-pane.js:453 > + const select = element('select', > + parameter.options.map((option) => element('option', > + {value: option, selected: option == parameter.value}, option))); Can we just do this in one or two lines?
<rdar://problem/39199641>