diff --git a/.changeset/odd-yaks-rush.md b/.changeset/odd-yaks-rush.md new file mode 100644 index 00000000000..2775d2d89c4 --- /dev/null +++ b/.changeset/odd-yaks-rush.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/kas": patch +--- + +Expressions are now compared more thoroughly. Now we always check that the expressions evaluate the same with all variables bound to -1, 0, and 1. We also check more randomly-chosen values: 28 instead of 12. diff --git a/packages/kas/src/__tests__/comparing.test.ts b/packages/kas/src/__tests__/comparing.test.ts index bca948f0988..b91de2b306e 100644 --- a/packages/kas/src/__tests__/comparing.test.ts +++ b/packages/kas/src/__tests__/comparing.test.ts @@ -136,7 +136,8 @@ describe("comparing", () => { expect(forms2[0]).toEqualExpr(form); }); - expect("x").toEqualExpr("xy/y"); + expect("x").toEqualExpr("x(y^2 + 1)/(y^2 + 1)"); + expect("x").not.toEqualExpr("x(y^2 + 1)/(y^2 + 2)"); expect("e^x").toEqualExpr("e^x"); expect("e^x").not.toEqualExpr("e^x + 1"); @@ -268,8 +269,8 @@ describe("comparing", () => { test("simplify can't yet handle these", () => { expect("sin(x + 2pi)").toEqualExpr("sin(x)"); expect("y = sin(x + 2pi)").toEqualExpr("y = sin(x)"); - expect("sin^2(x)+cos^2(x)").toEqualExpr("x/x"); - expect("y = sin^2(x)+cos^2(x)").toEqualExpr("y = x/x"); + expect("sin^2(x)+cos^2(x)").toEqualExpr("1"); + expect("y = sin^2(x)+cos^2(x)").toEqualExpr("y = 1"); }); test("partially evaluating functions", () => { diff --git a/packages/kas/src/nodes.ts b/packages/kas/src/nodes.ts index 129947a9982..0dfae4b67ab 100644 --- a/packages/kas/src/nodes.ts +++ b/packages/kas/src/nodes.ts @@ -56,7 +56,6 @@ const randomFloat = function (min: number, max: number) { }; /* constants */ -var ITERATIONS = 12; var TOLERANCE = 9; // decimal places // NOTE(kevinb): _.partition exists in a more recent version of underscore. @@ -437,40 +436,62 @@ abstract class Expr { return false; } - // Compare at a set number (currently 12) of points to determine - // equality. + + // To compare expressions, we evaluate them for a set of randomly + // chosen variable values. If the expressions evaluate the same for + // all variable bindings, we consider them equal. // - // `range` (and `vars`) is the only variable that varies through the - // iterations. For each of range = 10, 100, and 1000, each random - // variable is picked from (-range, range). + // We make sure to check integer variable values as well as floats. + // This is because expressions like (-2)^x are common but result + // in NaN when evaluated in JS with non-integer values of x. + // Without this, (-2)^x and (-2)^(x+1) both end up always being NaN + // and thus equivalent. With this, the most common failure case is + // avoided. However, less common cases such as (-2)^(x+0.1) and + // (-2)^(x+1.1) will still both evaluate to NaN and result in a + // false positive. // - // Note that because there are 12 iterations and three ranges, each - // range is checked four times. - for (var i = 0; i < ITERATIONS; i++) { + // Note that the above is only true in vanilla JS Number-land, + // which has no concept of complex numbers. The solution is simple: + // Integrate a library for handling complex numbers. + const randomValueGenerators = [ + // magnitude <= 1: + floatGenerator(0, 1), + floatGenerator(-1, 0), + // magnitude <= 10: + intGenerator(0, 10), + intGenerator(-10, 0), + floatGenerator(0, 10), + floatGenerator(-10, 0), + // magnitude <= 100: + intGenerator(0, 100), + intGenerator(-100, 0), + floatGenerator(0, 100), + floatGenerator(-100, 0), + // magnitude <= 1000: + intGenerator(0, 1000), + intGenerator(-1000, 0), + floatGenerator(0, 1000), + floatGenerator(-1000, 0), + ] + + const variableValueGenerators = [ + // Always check x = -1, x = 0, and x = 1. + // Requested by content creators: + // https://khanacademy.slack.com/archives/CJDRXTGQ7/p1776180198083239?thread_ts=1776096483.309839&cid=CJDRXTGQ7 + constantGenerator(-1), + constantGenerator(0), + constantGenerator(1), + // Double up the random value generators so we check two int + // values and two float values in each order of magnitude. + ...randomValueGenerators, + ...randomValueGenerators, + ]; + + for (const generateValue of variableValueGenerators) { var vars = {}; - // One third total iterations each with range 10, 100, and 1000 - var range = Math.pow(10, 1 + Math.floor((3 * i) / ITERATIONS)); - - // Half of the iterations should only use integer values. - // This is because expressions like (-2)^x are common but result - // in NaN when evaluated in JS with non-integer values of x. - // Without this, (-2)^x and (-2)^(x+1) both end up always being NaN - // and thus equivalent. With this, the most common failure case is - // avoided. However, less common cases such as (-2)^(x+0.1) and - // (-2)^(x+1.1) will still both evaluate to NaN and result in a - // false positive. - // - // Note that the above is only true in vanilla JS Number-land, - // which has no concept of complex numbers. The solution is simple: - // Integrate a library for handling complex numbers. - // - var useFloats = i % 2 === 0; - _.each(varList, function (v) { - vars[v] = useFloats - ? randomFloat(-range, range) - : _.random(-range, range); + vars[v] = generateValue(); }); let equal; @@ -3666,6 +3687,18 @@ export class Unit extends Sym { } } +function constantGenerator(value: number): () => number { + return () => value; +} + +function floatGenerator(min: number, max: number): () => number { + return () => randomFloat(min, max); +} + +function intGenerator(min: number, max: number): () => number { + return () => _.random(min, max); +} + // If possible, replace unit prefixes with a multiplication. // // "g" -> Unit("g")