Make expression comparison more thorough#3503
Conversation
… 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.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +100 B (+0.02%) Total Size: 499 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (6835eb5) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3503If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3503If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3503 |
SonicScrewdriver
left a comment
There was a problem hiding this comment.
From a code standpoint, I think this looks like a great improvement and all the tests pass. Thank you for including such detailed comments!
It wouldn't be a bad plan to get the Math Content team to double check that everything looks right, but I'm happy to approve myself!
| 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"); |
There was a problem hiding this comment.
This seems much clearer!
| 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 |
There was a problem hiding this comment.
This was honestly very useful, but I wonder if we want to permanently include our slack link in our open source repo. Maybe we could add the ticket ID and a comment on the ticket with the slack link?
This is truly just a nit / musing, so feel free to disregard at your discretion.
| } | ||
|
|
||
| function intGenerator(min: number, max: number): () => number { | ||
| return () => _.random(min, max); |
There was a problem hiding this comment.
I'd love it if we could clear out some of the underscore while we're in here, but that juice might not be worth the squeeze right now.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus-core@25.0.0 ### Major Changes - [#3511](#3511) [`15b0193db5`](15b0193) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused `static` field from PerseusCSProgramWidgetOptions. Callers should update test data that constructs `PerseusCSProgramWidgetOptions` to remove the static field. ### Minor Changes - [#3441](#3441) [`de45286302`](de45286) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of new Vector graph and subcomponents. - [#3433](#3433) [`b4bb6e2f42`](b4bb6e2) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of initial types and stubs for Vector graph ### Patch Changes - [#3434](#3434) [`de2dda0258`](de2dda0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Implementation of state management logic for new Vector graph - Updated dependencies \[[`a5b9105c28`](a5b9105)]: - @khanacademy/kas@2.2.2 ## @khanacademy/perseus@77.3.0 ### Minor Changes - [#3441](#3441) [`de45286302`](de45286) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of new Vector graph and subcomponents. - [#3433](#3433) [`b4bb6e2f42`](b4bb6e2) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of initial types and stubs for Vector graph - [#3494](#3494) [`8fb79829d0`](8fb7982) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Interactive Graph: change asymptote line to dashed for both exponential and logarithm based on user feedback ### Patch Changes - [#3523](#3523) [`c89cdbe2aa`](c89cdbe) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Ensure the default coords for Exponential and Logarithm are slightly further away from the asymptote. - [#3508](#3508) [`16f7f77ba1`](16f7f77) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) | Give explore modal launcher a label saying it has description - [#3496](#3496) [`4d923417cd`](4d92341) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add fallback label to Numeric Inputs and a linter warning for missing labels in the editor. - [#3511](#3511) [`15b0193db5`](15b0193) Thanks [@benchristel](https://github.com/benchristel)! - Remove unused `static` field from PerseusCSProgramWidgetOptions. Callers should update test data that constructs `PerseusCSProgramWidgetOptions` to remove the static field. - [#3504](#3504) [`b8178b52e7`](b8178b5) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (a11y) | Add aria-describedby to Explore Image modal - [#3488](#3488) [`3abc5e8277`](3abc5e8) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - [Free Response] Add visual regression tests - [#3493](#3493) [`11742c2cff`](11742c2) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Implementing bug fix for jumping MovableLines in the Correct Answer graph in the editor - [#3483](#3483) [`7794943ec7`](7794943) Thanks [@nishasy](https://github.com/nishasy)! - [ColorSync] Numeric Input - update visual regression tests - [#3434](#3434) [`de2dda0258`](de2dda0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Implementation of state management logic for new Vector graph - Updated dependencies \[[`de45286302`](de45286), [`4d923417cd`](4d92341), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`d3ef4dbcc2`](d3ef4db), [`a5b9105c28`](a5b9105), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 - @khanacademy/perseus-linter@4.9.5 - @khanacademy/perseus-score@8.7.0 - @khanacademy/kas@2.2.2 - @khanacademy/keypad-context@3.2.44 - @khanacademy/kmath@2.4.2 - @khanacademy/math-input@26.4.15 ## @khanacademy/perseus-editor@30.4.0 ### Minor Changes - [#3441](#3441) [`de45286302`](de45286) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of new Vector graph and subcomponents. - [#3443](#3443) [`a3396604a7`](a339660) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of new Vector Graph Editor - [#3433](#3433) [`b4bb6e2f42`](b4bb6e2) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Creation of initial types and stubs for Vector graph - [#3492](#3492) [`883133a28f`](883133a) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Add preview data sanitizer to strip non-serializable values before postMessage ### Patch Changes - [#3522](#3522) [`19911cc966`](19911cc) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Label absolute-value graph start coordinates as "Vertex" and "Arm" in the editor instead of "Point 1" and "Point 2". - [#3505](#3505) [`1ab914fc41`](1ab914f) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (CX) | Add warning for large images - [#3530](#3530) [`b5e918e8b3`](b5e918e) Thanks [@nishasy](https://github.com/nishasy)! - [Image] Update copy of recalculate button in editor - [#3434](#3434) [`de2dda0258`](de2dda0) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Implementation of state management logic for new Vector graph - Updated dependencies \[[`de45286302`](de45286), [`c89cdbe2aa`](c89cdbe), [`16f7f77ba1`](16f7f77), [`4d923417cd`](4d92341), [`15b0193db5`](15b0193), [`b8178b52e7`](b8178b5), [`b4bb6e2f42`](b4bb6e2), [`d3ef4dbcc2`](d3ef4db), [`3abc5e8277`](3abc5e8), [`a5b9105c28`](a5b9105), [`11742c2cff`](11742c2), [`7794943ec7`](7794943), [`de2dda0258`](de2dda0), [`8fb79829d0`](8fb7982)]: - @khanacademy/perseus@77.3.0 - @khanacademy/perseus-core@25.0.0 - @khanacademy/perseus-linter@4.9.5 - @khanacademy/perseus-score@8.7.0 - @khanacademy/kas@2.2.2 - @khanacademy/keypad-context@3.2.44 - @khanacademy/kmath@2.4.2 - @khanacademy/math-input@26.4.15 ## @khanacademy/perseus-score@8.7.0 ### Minor Changes - [#3442](#3442) [`d3ef4dbcc2`](d3ef4db) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Added ability to score Vector Interactive Graphs ### Patch Changes - Updated dependencies \[[`de45286302`](de45286), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`a5b9105c28`](a5b9105), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 - @khanacademy/kas@2.2.2 - @khanacademy/kmath@2.4.2 ## @khanacademy/kas@2.2.2 ### Patch Changes - [#3503](#3503) [`a5b9105c28`](a5b9105) Thanks [@benchristel](https://github.com/benchristel)! - 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. ## @khanacademy/keypad-context@3.2.44 ### Patch Changes - Updated dependencies \[[`de45286302`](de45286), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 ## @khanacademy/kmath@2.4.2 ### Patch Changes - Updated dependencies \[[`de45286302`](de45286), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 ## @khanacademy/math-input@26.4.15 ### Patch Changes - Updated dependencies \[[`de45286302`](de45286), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 - @khanacademy/keypad-context@3.2.44 ## @khanacademy/perseus-linter@4.9.5 ### Patch Changes - [#3496](#3496) [`4d923417cd`](4d92341) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Add fallback label to Numeric Inputs and a linter warning for missing labels in the editor. - Updated dependencies \[[`de45286302`](de45286), [`15b0193db5`](15b0193), [`b4bb6e2f42`](b4bb6e2), [`a5b9105c28`](a5b9105), [`de2dda0258`](de2dda0)]: - @khanacademy/perseus-core@25.0.0 - @khanacademy/kas@2.2.2 - @khanacademy/kmath@2.4.2
Summary:
Content creators reported a bug in an expression widget where the correct
answer was
-x / sqrt(1-x^2). You could put anything in the numerator and youranswer would (usually) be marked correct.
The bug happened because of the nondeterministic (and not very thorough) way
that we compared expressions. Previously, we selected 12 random variable
bindings and checked that the two expressions evaluated the same for all of
them. Some effort was made to cover different orders of magnitude, but simple
values like
-1,0, and1were not always checked.This meant that for expressions with a restricted domain, like
-x / sqrt(1 - x^2)(which is only defined in the interval[-1, 1]), we might check onlyvariable bindings for which the expression was not defined.
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.
Summary:
Issue: LEMS-4042
Test plan:
The expression in this article should be scored correctly:
https://www.khanacademy.org/devadmin/content/articles/arc-length-of-function-graphs-examples/x615e6443