-
Notifications
You must be signed in to change notification settings - Fork 364
[Interactive Graph] Add logarithm math utilities to kmath #3421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
66f26a1
4f8ea19
473ae00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@khanacademy/kmath": minor | ||
| --- | ||
|
|
||
| Add the logarithm math utilities to kmath for supporting Logarithm graph in Interactive Graph |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,16 @@ | |
| return [amplitude, angularFrequency, phase, verticalOffset]; | ||
| } | ||
|
|
||
| /** Coefficients for f(x) = a·ln(b·x + c). */ | ||
| export type LogarithmCoefficient = { | ||
| /** Vertical scale factor. */ | ||
| a: number; | ||
| /** Horizontal scale factor. */ | ||
| b: number; | ||
| /** Horizontal shift (determines vertical asymptote at x = −c/b). */ | ||
| c: number; | ||
| }; | ||
|
|
||
| /** Coefficients for f(x) = a·eᵇˣ + c. */ | ||
| export type ExponentialCoefficient = { | ||
| /** Vertical scale factor (amplitude). */ | ||
|
|
@@ -81,7 +91,56 @@ | |
| return {a, b, c}; | ||
| } | ||
|
Comment on lines
91
to
92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 This is a pre-existing bug in getExponentialCoefficients (not modified by this PR): when both input points share the same y-coordinate but neither lies on the asymptote, the function returns a degenerate {a, b:0, c} constant function instead of undefined. The fix is to add a b===0 guard to getExponentialCoefficients, mirroring the bExp===0 guard already present in the new getLogarithmCoefficients. Extended reasoning...What the bug is and how it manifests: The specific code path: Why existing guards don't prevent it: Step-by-step proof:
This represents Impact and fix: |
||
|
|
||
| /** | ||
| * Returns the coefficients {a, b, c} for f(x) = a·ln(b·x + c) given two | ||
| * points on the curve and the x-value of the vertical asymptote. | ||
| * | ||
| * Uses the inverse exponential approach: flip each coordinate (x, y) → (y, x), | ||
| * compute exponential coefficients from the flipped points, then invert. | ||
| * | ||
| * Returns undefined if the inputs are geometrically invalid (same y, a point | ||
| * on the asymptote, or points on opposite sides of the asymptote). | ||
| */ | ||
| export function getLogarithmCoefficients( | ||
| coords: ReadonlyArray<Coord>, | ||
| asymptote: number, | ||
| ): LogarithmCoefficient | undefined { | ||
| const p1 = coords[0]; | ||
| const p2 = coords[1]; | ||
|
|
||
| if (p1[1] === p2[1]) { | ||
| return; | ||
| } // same y makes bExp undefined | ||
| if (p1[0] === asymptote || p2[0] === asymptote) { | ||
| return; | ||
| } // point on asymptote | ||
|
|
||
| // Flip coordinates: treat logarithm as inverse exponential | ||
| const p1Flipped: Coord = [p1[1], p1[0]]; | ||
| const p2Flipped: Coord = [p2[1], p2[0]]; | ||
| const cExp = asymptote; | ||
|
|
||
| const ratio = (p1Flipped[1] - cExp) / (p2Flipped[1] - cExp); | ||
| if (ratio <= 0) { | ||
| return; | ||
| } // points on opposite sides of asymptote | ||
|
|
||
| const bExp = Math.log(ratio) / (p1Flipped[0] - p2Flipped[0]); | ||
| const aExp = (p1Flipped[1] - cExp) / Math.exp(bExp * p1Flipped[0]); | ||
|
|
||
| if (!isFinite(bExp) || !isFinite(aExp) || aExp === 0 || bExp === 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Invert exponential coefficients to get logarithm coefficients | ||
| const a = 1 / bExp; | ||
| const b = 1 / aExp; | ||
| const c = -cExp / aExp; | ||
|
|
||
| return {a, b, c}; | ||
| } | ||
|
|
||
| // p1 is the inflection point (where tan = 0, i.e. the curve crosses | ||
|
Check warning on line 143 in packages/kmath/src/coefficients.ts
|
||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // through its vertical offset). p2 is a quarter-period away and | ||
| // determines the amplitude and period of the tangent function. | ||
| export function getTangentCoefficients( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The JSDoc for
getLogarithmCoefficientsomits the same-x-coordinate case from its list of invalid inputs, and there is no test covering this code path, unlike the siblinggetExponentialCoefficientswhich documents and tests its analogous same-x case. The function does correctly returnundefinedwhenp1[0]===p2[0](via thebExp===0guard), but this behavior is undocumented and untested.Extended reasoning...
What the bug is:
The
getLogarithmCoefficientsJSDoc (coefficients.ts ~line 100) describes three invalid-input cases: "same y, a point on the asymptote, or points on opposite sides of the asymptote." The same-x-coordinate case is conspicuously absent, and there is no corresponding test in coefficients.test.ts (lines 158–183), unlikegetExponentialCoefficientswhich has an explicit test titled "returns undefined when both points share the same x-coordinate" (line ~56).Why the refutation misses the point:
The refutation correctly notes the function is functionally correct — this is not in dispute. The issue is a documentation and test coverage gap. Good software practices require that: (1) documented behavior matches actual behavior, (2) all meaningful code paths are tested, and (3) sibling functions follow consistent patterns. The refutation argues "missing tests or incomplete JSDoc are code quality suggestions, not bugs" — but by the same logic, nothing is ever a bug if the computation returns an answer. The
getExponentialCoefficientsfunction sets the expectation for how these functions should be documented and tested.Step-by-step proof of the undocumented code path:
Given
p1=[3,1],p2=[3,5],asymptote=0:p1[1] \!== p2[1](1 ≠ 5) — same-y guard passesp1[0] \!== asymptote && p2[0] \!== asymptote(3 ≠ 0) — asymptote guard passesp1Flipped=[1,3],p2Flipped=[5,3]ratio = (3-0)/(3-0) = 1— opposite-sides guard passes (1 > 0)bExp = Math.log(1) / (1-5) = 0 / (-4) = 0bExp === 0guard fires → returnsundefined✓The function returns correctly, but this path is never exercised by any test, and the JSDoc gives no hint this case is handled.
How to fix it:
expect(getLogarithmCoefficients([[3,1],[3,5]], 0)).toBeUndefined()— mirroring the existinggetExponentialCoefficientstest.Impact:
Purely a documentation and test coverage gap. No functional impact on callers. However, downstream PRs 3–6 in this series will all rely on
getLogarithmCoefficients, so establishing complete test coverage now is valuable before the function is widely consumed.