Fix the two piecewise-linear regression calculation #67
Conversation
|
Said, I'm hoping Johnny and/or Victor will review this. If they are unable, I will. |
WebKit#66 The current implementation of the regression calculation has these flaws: 1. When processing (x[0], y[0]), L1 must be any line through (x0, y0) which meets L2 at a point (x’, y’) where x[0] < x' < x[1]. L1 has no error. 2. When processing (x[n - 2], y[n - 2]), L2 must be any line through (x[n - 1], y[n - 1]) which meets L1 at a point (x’, y’) where x[n - 2] < x' < x[n - 1]. L2 has no error. 3. The lambda calculation is incorrect. It includes a term called H which is equal to C - I. Looking at the algorithm of Kundu/Ubhaya, this should be just C. 4. lambda should to be used with calculating L1 and (1 - lambda) should to be used with calculating L2. Currently (1 - lambda) is used in calculating L1 and L2. 5. The current calculation has this condition if (t1 != t2) continue; This condition is almost always true even if t1 and t2 are essentiallyEqual.
shallawa
force-pushed
the
eng/fix-regression-calculation-issues
branch
from
a9cd222
to
b12b93d
Compare
There was a problem hiding this comment.
Hey Said,
I've spent a chunk of time going over this change and from my read thus far this looks like a good fix for the issues you pointed out in the description of this PR, nothing really material to comment on, but I did leave a few comments point out a few minor issues for you to consider.
The bigger question I have is whether you already have a view of how this performs relative to the current code, i.e. I'm assuming you've pushed a number of run results through this and done a side-by-side comparison already? If so, seeing the results of that in this PR/issue would certainly be helpful as there's a decent amount of complexity in how this is all calculated.
Thanks!
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright (C) 2015-2017 Apple Inc. All rights reserved. | |||
| * Copyright (C) 2015-2014 Apple Inc. All rights reserved. | |||
There was a problem hiding this comment.
Guessing this should be 2024 rather than 2014...
|
|
||
| _areEssentiallyEqual: function(n1, n2) { | ||
| const epsilon = 0.0001; | ||
| return Math.abs(n1 - n2) < epsilon; |
There was a problem hiding this comment.
Maybe add some rationale in a comment here for why 0.0001 is a reasonable epsilon? I'm assuming that was empirically determined, or is there more behind that choice?
|
|
||
| if (!this._areEssentiallyEqual(segment1.t, segment2.t)) { | ||
| // If segment1 and segment2 are not parallel, then they have to meet | ||
| // at complexity such that xp < complexity < x. |
| let a1 = 0, b1 = 0, c1 = 0, d1 = 0, h1 = 0, k1 = 0; | ||
| let a2 = 0, b2 = 0, c2 = 0, d2 = 0, h2 = 0, k2 = 0; | ||
|
|
||
| for (var j = 0; j < sortedSamples.length; ++j) { |
There was a problem hiding this comment.
Maybe use "let" in this loop as well?
smfr
added
the
non-trivial
#66
The current implementation of the regression calculation has these flaws:
When processing (x[0], y[0]), L1 must be any line through (x[0], y[0]) which meets L2 at a point (x’, y’) where x[0] < x' < x[1]. L1 has no error.
When processing (x[n - 2], y[n - 2]), L2 must be any line through (x[n - 1], y[n - 1]) which meets L1 at a point (x’, y’) where x[n - 2] < x' < x[n - 1]. L2 has no error.
The lambda calculation is incorrect. It includes a term called H which is equal to C - I. Looking at the algorithm of Kundu/Ubhaya, this should be just C.
lambda should to be used with calculating L1 and (1 - lambda) should to be used with calculating L2. Currently (1 - lambda) is used in calculating L1 and L2.
The current calculation has this condition if (t1 != t2) continue; This condition is almost always true even if t1 and t2 are essentiallyEqual.