core: ensure log-normal score is always in correct range #13392
Conversation
| @@ -88,7 +88,7 @@ module.exports = { | |||
| ], | |||
| parser: '@typescript-eslint/parser', | |||
| parserOptions: { | |||
| ecmaVersion: 2019, | |||
| ecmaVersion: 2020, | |||
There was a problem hiding this comment.
needed for BigInt64Array (available since Node 10.8) in a test. If anyone doesn't feel good updating this for the whole project, I can also just add BigInt64Array as an eslint global to just the test file
| @@ -28,37 +34,6 @@ function erf(x) { | |||
| return sign * (1 - y * Math.exp(-x * x)); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Not clear why this wasn't deleted in #10715. Possibility someone was using it? Hard to let go of our old friend the podr?
Searching github the only references I can find that aren't forks of this file are references to forks of this file from four year old forks of Lighthouse, so I think we're good to delete.
Farewell podr! You'll live on as an increasingly mystifying reference in our desmos graphs.
| const INVERSE_ERFC_ONE_FIFTH = 0.9061938024368232; | ||
|
|
||
| // Shape (σ) is `log(p10/median) / (sqrt(2)*erfc^-1(2 * 1/10))` and | ||
| // Shape (σ) is `|log(p10/median) / (sqrt(2)*erfc^-1(1/5))|` and |
There was a problem hiding this comment.
took me a little while to remember what the // negate to keep σ positive below was referring to, and it was that σ needs to be abs, so making that a little clearer
| const xRatio = Math.max(Number.MIN_VALUE, value / median); // value and median are > 0, so is ratio. | ||
| const xLogRatio = Math.log(xRatio); | ||
| const p10Ratio = Math.max(Number.MIN_VALUE, p10 / median); // p10 and median are > 0, so is ratio. | ||
| const p10LogRatio = -Math.log(p10Ratio); // negate to keep σ positive. |
There was a problem hiding this comment.
this is unchanged except the extra Math.max() calls that ensure that these values stay reasonable for some extreme cases where value / median or p10 / median underflow to 0 and NaN can result below.
It's extremely unlikely anyone would ever pick values that would trigger that, but good to have it handled.
Interestingly it appears that if a double a and b are greater than 0 and a < b, then a / b < 1, which is nice. I thought maybe rounding in the last place could sometimes bump it up to 1, but that appears to never happen (though I haven't come across a proof of this).
There was a problem hiding this comment.
Interestingly it appears that if a double a and b are greater than 0 ...
How did you come to this conclusion? Once you go above any value over MAX_SAFE_INTEGER I find this to be not true.
for (let x = -100; x < 1000; x++) console.log(x, (Number.MAX_SAFE_INTEGER - 1 + x) / (Number.MAX_SAFE_INTEGER + x));
There was a problem hiding this comment.
Ah, I suppose I am failing to ensure that each integer has a unique repr. in the double format.
There was a problem hiding this comment.
right, strictly a < b
| // Failing. Clamp to [0, 0.5). | ||
| score = Math.max(0, Math.min(MAX_FAILING_SCORE, complementaryPercentile)); | ||
| } | ||
| return score; |
There was a problem hiding this comment.
I wish we had Math.clamp !
| const big64 = new BigInt64Array(f64.buffer); | ||
| big64[0] -= 1n; | ||
| return f64[0]; | ||
| } |
There was a problem hiding this comment.
hello 911 i'd like to report a bit hacker
The numerical accuracy part of #13316, laying the foundation for the rest.
As mentioned there, currently
getLogNormalScore({p10: 200, median: 600}, 200) === 0.8999999314038525, which is < 0.9, so wouldn't be considered passing, even though 200 is ≥ the p10 threshold and so should be passing. In practice the scores are rounded to two decimal places so this change won't be observable, but if/when we change how the scores are rounded, this will be a problem.Also, it's just wrong and should be fixed :)
This change ensures that for raw scores (before rounding):
numericValueis ≤ thep10control point, then the audit score will be ≥ 0.9numericValueis > thep10control point, then the audit score will be < 0.9numericValueis ≤ themediancontrol point, then the audit score will be ≥ 0.5numericValueis > themediancontrol point, then the audit score will be < 0.5This isn't changing anything drastically, it's more or less nudging the few values that were on the wrong side of the score thresholds to the correct one. For example, for TBT (p10 200, median 600):
Before change
The TBT score finally hits 0.9 at a
numericValueof 199.99993298607299, going above 0.9 at 199.9999329860729.The current score hits 0.9 for TBT values less than a tenth of a microsecond faster than 200ms, so in practice this is only a change for sites at a TBT of 200ms. The other metrics have changes at a similar magnitude; this is more or less correcting the raw scores of values at exactly the p10 control points.
After change
The scores at
numericValue ≤ 200are Math.maxed to 0.9 while the others are unaffected. Once the score goes above 0.9 (still at 199.9999329860729) it proceeds as before, so it's really just this tiny range that has its scores changed from e.g. 0.8999999314038525 to 0.9.For those that care, the scoring function is as monotonic as it's ever been†, there's just a bigger first derivative discontinuity.
†have not verified the Abramowitz and Stegun approximation