Conversation
…nicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util
…ngentCoefficients and remove the duplicate code in grapher-util
🗄️ Schema Change: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0005d95) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3406If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3406If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3406 |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +29 B (+0.01%) Total Size: 494 kB
ℹ️ View Unchanged
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
There was a problem hiding this comment.
The deduplication rationale is clear and well-documented, but the canonicalization behavior change (dropping the a > 0 guarantee) affects tangent scoring correctness and warrants a human look to confirm no existing content is impacted.
Extended reasoning...
Overview
This PR removes canonicalTangentCoefficients and Tangent.areEqual from grapher-util.ts in perseus-core, replacing their usage in score-grapher.ts with kmath's coefficients.getTangentCoefficients and geometry.canonicalTangentCoefficients. Two tangent scoring tests are added. The changeset marks both perseus-core and perseus-score as minor bumps.
Security Risks
No security-sensitive code is touched. No auth, crypto, or permission logic is involved.
Level of Scrutiny
The changes are confined to tangent-curve scoring logic, which is production-critical — it directly determines whether a student answer is marked correct or incorrect. The PR author explicitly documents a behavior change: the old canonicalTangentCoefficients enforced both a > 0 and b > 0; kmath's version only enforces b > 0. The author argues this is safe because a < 0, b > 0 represents a genuinely different curve and existing exercises are unlikely to rely on the legacy normalization. That reasoning is plausible, but verifying it requires confidence that no stored rubric was authored under conditions where the old symmetric normalization masked a sign mismatch — which is hard to confirm without content analysis.
Other Factors
The two bugs reported by the automated system are minor: one is a nit about a test title naming an internal library function, and the other is a pre-existing degenerate-input edge case that this PR actually improves (replacing an infinite-loop hang with a quiet 'incorrect' result). Neither blocks the overall correctness of the approach. The circular-dependency rationale (kmath depends on perseus-core, so the kmath calls must live in perseus-score) is sound.
There was a problem hiding this comment.
The feedback from the previous review has been addressed — test title fixed and degenerate-input case now tested. Remaining findings are nits with no runtime impact. LGTM.
Extended reasoning...
Overview
This PR removes duplicate tangent coefficient logic from grapher-util.ts (perseus-core) and replaces it with coefficients.getTangentCoefficients and geometry.canonicalTangentCoefficients from kmath. Four files are modified: the changeset, grapher-util.ts, score-grapher.ts, and score-grapher.test.ts. The scoring path for tangent is now handled entirely via kmath, with a new isFinite guard catching degenerate same-x inputs.
Security risks
None. This is internal scoring math with no auth, crypto, or data-exposure surface.
Level of scrutiny
Scoring code warrants care since it affects whether learners receive credit. The PR description is thorough about the canonicalization behavior change (kmath only guarantees b > 0, not a > 0), and explains why the practical impact is negligible. The isFinite guard correctly converts degenerate inputs (Infinity from same-x coords) into an invalid result, and a new test confirms this. The previous run left comments about the test title and the degenerate-input case; both were addressed in commit 72a44e1.
Other factors
The two new nit-level findings are: (1) optional chaining on a non-nullable TangentCoefficient return — zero runtime impact, style-only; (2) changeset description mentioning Interactive Graph when only Grapher changed — affects only the changelog, not code. Neither warrants blocking. Test coverage is good: correct, incorrect, and degenerate cases are all covered.
| "@khanacademy/perseus-score": minor | ||
| --- | ||
|
|
||
| Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util |
There was a problem hiding this comment.
🟡 The changeset description claims changes were made "for both Grapher and Interactive Graph widgets", but this PR only modifies Grapher files — the Interactive Graph widget (score-interactive-graph.ts) is untouched. Update the description to say "for the Grapher widget" to avoid misleading changelog consumers into thinking Interactive Graph scoring behavior changed in this release.
Extended reasoning...
What the bug is
The changeset file .changeset/cool-shirts-thank.md at line 6 reads: "Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util". This claim is factually inaccurate — only the Grapher widget was changed in this PR.
The specific code evidence
The PR diff modifies exactly four files: .changeset/cool-shirts-thank.md, packages/perseus-core/src/utils/grapher-util.ts, packages/perseus-score/src/widgets/grapher/score-grapher.test.ts, and packages/perseus-score/src/widgets/grapher/score-grapher.ts. Every changed file is either the changeset itself or lives under the grapher/ directory. No file from the interactive-graph/ directory appears anywhere in the diff.
Why the description is wrong
score-interactive-graph.ts already imported and used canonicalTangentCoefficients from kmath (geometry module) before this PR was opened — it was already on kmath. The PR description itself is accurate ("[Grapher] Use kmath canonicalTangentCoefficients..."), but the changeset body contradicts it by mentioning Interactive Graph. The verifier refutation noting bug_005 is a duplicate of bug_001 does not dispute the existence of the inaccuracy — it simply consolidates two reports of the same issue.
Impact
Changeset descriptions feed directly into the public CHANGELOG.md. A downstream consumer reading the release notes would reasonably conclude that Interactive Graph scoring behavior changed in this release, and might investigate regressions or compatibility issues in that widget unnecessarily. The inaccuracy does not affect runtime behavior, making this a nit — but accurate changelogs are important for library consumers.
How to fix it
Change line 6 of .changeset/cool-shirts-thank.md from: Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util to: Use kmath canonicalTangentCoefficients for the Grapher widget and remove the duplicate code in grapher-util
Step-by-step proof
- Open
.changeset/cool-shirts-thank.mdline 6 — it claims "both Grapher and Interactive Graph widgets" were changed. - Check the PR diff — only 4 files are modified, all in
grapher-util.tsorgrapher/subdirectory. - Confirm
score-interactive-graph.tsis absent from the diff entirely. - Search the pre-PR codebase:
score-interactive-graph.tsalready importscanonicalTangentCoefficientsfrom kmath geometry at line 24 — it predates this PR. - Conclusion: the Interactive Graph widget was already using kmath before this PR; this PR only migrated the Grapher widget. The changeset description should reflect only the Grapher change.
There was a problem hiding this comment.
This is okay, i want it to be explicit the connection between the two widgets with the common function they used.
There was a problem hiding this comment.
LGTM — prior feedback addressed, logic is correct, and the new tests cover the degenerate same-x case.
Extended reasoning...
Overview
This PR removes the duplicate canonicalTangentCoefficients and Tangent.areEqual from grapher-util.ts in perseus-core, and replaces their usage in score-grapher.ts with kmath equivalents. Four files are touched: the changeset, grapher-util.ts, score-grapher.ts, and its test file.
Security risks
No security-sensitive code is involved. The changes affect math/scoring logic only.
Level of scrutiny
This is a focused refactor with clear motivation (eliminating code duplication, preventing circular dependencies). The behavior change (kmath only guarantees b > 0, not a > 0) is correctly documented and is mathematically sound — the old normalization was actually incorrect for tangent. The new isFinite guard in getCoefficientsByType correctly handles the degenerate same-x control point case that previously caused an infinite loop.
Other factors
All actionable feedback from prior reviews has been addressed: the test title no longer references implementation details, the coeffs?.some() optional chaining on a non-nullable type was corrected to coeffs.some(), and a new test explicitly covers the degenerate same-x tangent input case. The flagged sinusoid infinite loop bug is pre-existing and not introduced by this PR. The changeset description wording was kept per explicit author preference. Test coverage is solid.
| return [amplitude, angularFrequency, phase, verticalOffset]; | ||
| } | ||
|
|
||
| function canonicalTangentCoefficients(coeffs: any) { | ||
| // For a curve of the form f(x) = a * Tan(b * x - c) + d, | ||
| // this function ensures that a, b > 0, and c is its | ||
| // smallest possible positive value. | ||
| let amplitude = coeffs[0]; | ||
| let angularFrequency = coeffs[1]; | ||
| let phase = coeffs[2]; | ||
| const verticalOffset = coeffs[3]; | ||
|
|
||
| // Guarantee a > 0 | ||
| if (amplitude < 0) { | ||
| amplitude *= -1; | ||
| angularFrequency *= -1; | ||
| phase *= -1; | ||
| } | ||
|
|
||
| const period = Math.PI; | ||
| // Guarantee b > 0 | ||
| if (angularFrequency < 0) { | ||
| angularFrequency *= -1; | ||
| phase *= -1; | ||
| phase += period / 2; | ||
| } | ||
|
|
||
| // Guarantee c is smallest possible positive value | ||
| while (phase > 0) { | ||
| phase -= period; | ||
| } | ||
| while (phase < 0) { | ||
| phase += period; | ||
| } | ||
|
|
||
| return [amplitude, angularFrequency, phase, verticalOffset]; | ||
| } | ||
|
|
||
| const PlotDefaults = { | ||
| areEqual: function ( |
There was a problem hiding this comment.
🟣 Pre-existing issue: when both sinusoid control points share the same x-coordinate, Sinusoid.getCoefficients computes b = Math.PI / (2 * 0) = Infinity with no guard and returns a non-null array, bypassing scoreGrapher's null check. Sinusoid.areEqual then calls canonicalSineCoefficients with phase = Infinity, where the while (phase > 0) { phase -= period; } loop runs forever, hanging the browser. The PR explicitly fixed this exact pattern for tangent by adding an isFinite guard in getCoefficientsByType, but the analogous sinusoid branch was left unchanged.
Extended reasoning...
What the bug is and how it manifests
When a user submits a sinusoid graph with both control points at the same x-coordinate (e.g., [[1, 0], [1, 2]]), Sinusoid.getCoefficients in grapher-util.ts computes b = Math.PI / (2 * (p2[0] - p1[0])) = Math.PI / 0 = Infinity. With p1[0] != 0, it further computes c = p1[0] * Infinity = Infinity. The function returns [a, Infinity, Infinity, d] — a non-null array — with no guard for this degenerate case.
The specific code path that triggers it
In score-grapher.ts, getCoefficientsByType routes sinusoid to GrapherUtil.functionForType("sinusoid").getCoefficients(data.coords), which calls Sinusoid.getCoefficients. Unlike the new tangent branch (lines 26-31) which now checks coeffs.some((c) => !isFinite(c)), the sinusoid branch has no such guard and returns the [a, Infinity, Infinity, d] array directly. The null check at line 81 (if (guessCoeffs == null || correctCoeffs == null)) is bypassed. GrapherUtil.functionForType("sinusoid").areEqual(guessCoeffs, correctCoeffs) is then called, which invokes Sinusoid.areEqual, which calls canonicalSineCoefficients([a, Infinity, Infinity, d]).
Why existing code does not prevent the infinite loop
canonicalSineCoefficients (grapher-util.ts lines 23-56) receives phase = Infinity. After the amplitude/frequency normalization, it reaches: while (phase > 0) { phase -= period; } where period = 2 * Math.PI. Since Infinity > 0 is always true and Infinity - 2*PI = Infinity, this loop never terminates, hanging the browser tab entirely.
Why this is pre-existing and not introduced by this PR
The PR does not touch Sinusoid.getCoefficients or canonicalSineCoefficients. The sinusoid code path was unchanged. However, this PR is directly relevant because: (1) the PR description explicitly identifies this exact while (phase > 0) infinite-loop pattern as the motivation for the tangent fix, noting it "would infinitely loop for phase = Infinity when p1[0] != 0, hanging the browser entirely"; (2) the PR added a targeted isFinite guard in getCoefficientsByType for tangent (line 29); (3) the identical fix was not applied to the sinusoid branch, leaving the browser-hanging loop reachable.
Step-by-step proof
- User submits sinusoid with coords
[[1, 0], [1, 2]](same x = 1). getCoefficientsByTypecallsSinusoid.getCoefficients([[1, 0], [1, 2]]):a = 2,b = Math.PI / (2 * 0) = Infinity,c = 1 * Infinity = Infinity,d = 0. Returns[2, Infinity, Infinity, 0].- No
isFiniteguard exists in the sinusoid branch; the array is returned as-is.guessCoeffsis[2, Infinity, Infinity, 0], not null. - Null check at line 81 passes.
Sinusoid.areEqual([2, Infinity, Infinity, 0], ...)is called. canonicalSineCoefficients([2, Infinity, Infinity, 0]): amplitude=2>0 so no flip; angularFrequency=Infinity>0 so no flip;phase = Infinity. Reacheswhile (Infinity > 0) { Infinity -= 2*PI; }— infinite loop, browser hangs.
How to fix it
Apply the same isFinite guard to the sinusoid branch in getCoefficientsByType, consistent with what was done for tangent. Alternatively, add a denom === 0 guard in Sinusoid.getCoefficients (similar to Linear.getCoefficients and AbsoluteValue.getCoefficients), or replace the while loops in canonicalSineCoefficients with modular arithmetic (as kmath does for canonicalTangentCoefficients).
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/[email protected] ### Major Changes - [#3416](#3416) [`3e1e498bf5`](3e1e498) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove trivial function `getWidgetsMapFromItemData` - access widgets via `.question.widgets` instead. ### Patch Changes - [#3388](#3388) [`3f091b70cb`](3f091b7) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (UX) | Upscale Graphies within image zoom view - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e), [`2d8df47440`](2d8df47)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Major Changes - [#3414](#3414) [`669ed4e6b1`](669ed4e) Thanks [@benchristel](https://github.com/benchristel)! - Remove `null` from the type of `PerseusItem.answerArea`. ### Minor Changes - [#3406](#3406) [`6267d162b4`](6267d16) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util ## @khanacademy/[email protected] ### Minor Changes - [#3406](#3406) [`6267d162b4`](6267d16) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e), [`2d8df47440`](2d8df47), [`3f091b70cb`](3f091b7), [`3e1e498bf5`](3e1e498)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#3419](#3419) [`2d8df47440`](2d8df47) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - changes return types of linter functions to not be readonly array; exports linterwarningitem - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: claude[bot], anakaren-rojas Required Reviewers: Approved By: anakaren-rojas Checks: ⏭️ 2 checks have been skipped, ✅ 6 checks were successful Pull Request URL: #3412
…cate code in grapher-util (#3406) ## Summary: Use kmath canonicalTangentCoefficients and remove the duplicate code in grapher-util - Removes canonicalTangentCoefficients from grapher-util.ts (perseus-core) and replaces its usage in the Grapher widget scoring with geometry.canonicalTangentCoefficients from kmath - Replaces Tangent.getCoefficients usage in score-grapher.ts with coefficients.getTangentCoefficients from kmath - Adds 2 new tangent scoring tests to score-grapher.test.ts This eliminates code duplication between grapher-util.ts and kmath, which were maintaining identical tangent coefficient extraction logic and similar canonicalization logic. kmath is now the single source of truth for tangent math. Why not import kmath into grapher-util.ts directly? kmath depends on perseus-core, so importing kmath into perseus-core would create a circular dependency. Instead, the Grapher widget scoring (score-grapher.ts in perseus-score, which already depends on kmath) now uses kmath functions directly for the tangent case. Canonicalization behavior change. The removed canonicalTangentCoefficients in grapher-util guaranteed both a > 0 and b > 0 using a phase += π/2 step. This is mathematically incorrect for tangent (tan(x + π/2) = -cot(x), not -tan(x)), but worked because areEqual applied the same normalization to both sides. kmath's version only guarantees b > 0, which is mathematically correct. The practical impact is negligible — a < 0, b > 0 represents a genuinely different curve, so existing exercises are unlikely to rely on the legacy normalization. Tangent.areEqual fallback. Removing the custom areEqual from the Tangent object means it inherits PlotDefaults.areEqual (plain approximateDeepEqual). This is safe because score-grapher.ts now handles tangent comparison directly with kmath's canonicalization — Tangent.areEqual is no longer called in the scoring path. Co-Authored by Claude Code (Opus) Issue: LEMS-3955 ## Test plan - pnpm tsc — no type errors - pnpm lint — no lint errors - pnpm prettier . --check — formatting clean - Existing score-grapher tests pass (7 tests) - New tangent scoring tests pass (correct + incorrect) - Existing grapher-util tests pass (31 tests) Author: ivyolamit Reviewers: claude[bot], ivyolamit, SonicScrewdriver, handeyeco Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful, ⚪️ 1 check is neutral Pull Request URL: #3406
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/[email protected] ### Major Changes - [#3416](#3416) [`3e1e498bf5`](3e1e498) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove trivial function `getWidgetsMapFromItemData` - access widgets via `.question.widgets` instead. ### Patch Changes - [#3388](#3388) [`3f091b70cb`](3f091b7) Thanks [@nishasy](https://github.com/nishasy)! - [Image] | (UX) | Upscale Graphies within image zoom view - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e), [`2d8df47440`](2d8df47)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Major Changes - [#3414](#3414) [`669ed4e6b1`](669ed4e) Thanks [@benchristel](https://github.com/benchristel)! - Remove `null` from the type of `PerseusItem.answerArea`. ### Minor Changes - [#3406](#3406) [`6267d162b4`](6267d16) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util ## @khanacademy/[email protected] ### Minor Changes - [#3406](#3406) [`6267d162b4`](6267d16) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Use kmath canonicalTangentCoefficients for both Grapher and Interactive Graph widgets and then remove the duplicate code in grapher-util ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e), [`2d8df47440`](2d8df47), [`3f091b70cb`](3f091b7), [`3e1e498bf5`](3e1e498)]: - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#3419](#3419) [`2d8df47440`](2d8df47) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - changes return types of linter functions to not be readonly array; exports linterwarningitem - Updated dependencies \[[`6267d162b4`](6267d16), [`669ed4e6b1`](669ed4e)]: - @khanacademy/[email protected] - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: claude[bot], anakaren-rojas Required Reviewers: Approved By: anakaren-rojas Checks: ⏭️ 2 checks have been skipped, ✅ 6 checks were successful Pull Request URL: #3412
2 participants
Summary:
Use kmath canonicalTangentCoefficients and remove the duplicate code in grapher-util
This eliminates code duplication between grapher-util.ts and kmath, which were maintaining identical tangent coefficient extraction logic and similar canonicalization logic. kmath is now the single source of truth for tangent math.
Why not import kmath into grapher-util.ts directly? kmath depends on perseus-core, so importing kmath into perseus-core would create a circular dependency.
Instead, the Grapher widget scoring (score-grapher.ts in perseus-score, which already depends on kmath) now uses kmath functions directly for the tangent case.
Canonicalization behavior change. The removed canonicalTangentCoefficients in grapher-util guaranteed both a > 0 and b > 0 using a phase += π/2 step. This is mathematically incorrect for tangent (tan(x + π/2) = -cot(x), not -tan(x)), but worked because areEqual applied the same normalization to both sides. kmath's version only guarantees b > 0, which is mathematically correct. The practical impact is negligible — a < 0, b > 0 represents a genuinely different curve, so existing exercises are unlikely to rely on the legacy normalization.
Tangent.areEqual fallback. Removing the custom areEqual from the Tangent object means it inherits PlotDefaults.areEqual (plain approximateDeepEqual). This is safe because score-grapher.ts now handles tangent comparison directly with kmath's canonicalization — Tangent.areEqual is no longer called in the scoring path.
Co-Authored by Claude Code (Opus)
Issue: LEMS-3955
Test plan