Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eight-stingrays-drum.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Add logarithm graph state management and reducer for supporting Logarithm graph in Interactive Graph
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,16 @@ class InteractiveGraphQuestionBuilder {
return this;
}

withLogarithm(options?: {
coords?: [Coord, Coord];
asymptote?: number;
startCoords?: [Coord, Coord];
startAsymptote?: number;
}): InteractiveGraphQuestionBuilder {
this.interactiveFigureConfig = new LogarithmGraphConfig(options);
return this;
}

withAbsoluteValue(options?: {
coords?: [Coord, Coord];
startCoords?: [Coord, Coord];
Expand Down Expand Up @@ -864,6 +874,46 @@ class ExponentialGraphConfig implements InteractiveFigureConfig {
}
}

class LogarithmGraphConfig implements InteractiveFigureConfig {
private coords?: [Coord, Coord];
private asymptote?: number;
private startCoords?: [Coord, Coord];
private startAsymptote?: number;

constructor(options?: {
coords?: [Coord, Coord];
asymptote?: number;
startCoords?: [Coord, Coord];
startAsymptote?: number;
}) {
this.coords = options?.coords;
this.asymptote = options?.asymptote;
this.startCoords = options?.startCoords;
this.startAsymptote = options?.startAsymptote;
}

correct(): PerseusGraphType {
return {
type: "logarithm",
coords: this.coords,
asymptote: this.asymptote,
};
}

graph(): PerseusGraphType {
return {
type: "logarithm",
startCoords:
this.startCoords != null
? {
coords: this.startCoords,
asymptote: this.startAsymptote ?? 0,
}
: undefined,
};
}
}

class TangentGraphConfig implements InteractiveFigureConfig {
private coords?: [Coord, Coord];
private startCoords?: [Coord, Coord];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ import {
sinusoidQuestionWithDefaultCorrect,
tangentQuestion,
tangentQuestionWithDefaultCorrect,
logarithmQuestion,
logarithmQuestionWithDefaultCorrect,
sinusoidWithPiTicks,
unlimitedPointQuestion,
unlimitedPolygonQuestion,
Expand Down Expand Up @@ -239,6 +241,7 @@ describe("Interactive Graph", function () {
quadratic: quadraticQuestion,
sinusoid: sinusoidQuestion,
tangent: tangentQuestion,
logarithm: logarithmQuestion,
"unlimited-point": pointQuestion,
"unlimited-polygon": polygonQuestion,
};
Expand All @@ -257,6 +260,7 @@ describe("Interactive Graph", function () {
quadratic: quadraticQuestionWithDefaultCorrect,
sinusoid: sinusoidQuestionWithDefaultCorrect,
tangent: tangentQuestionWithDefaultCorrect,
logarithm: logarithmQuestionWithDefaultCorrect,
"unlimited-point": pointQuestionWithDefaultCorrect,
"unlimited-polygon": polygonQuestionDefaultCorrect,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,23 @@ export const graphWithLabeledFunction: PerseusRenderer =
})
.build();

export const logarithmQuestion: PerseusRenderer =
interactiveGraphQuestionBuilder()
.withContent(
"**Graph $f(x) = \\log(x + 6)$ in the interactive widget.**\n\n[[☃ interactive-graph 1]]",
)
.withLogarithm({
coords: [
[-4, -3],
[-5, -7],
],
asymptote: -6,
})
.build();

export const logarithmQuestionWithDefaultCorrect: PerseusRenderer =
interactiveGraphQuestionBuilder().withLogarithm().build();

export const sinusoidWithPiTicks: PerseusRenderer =
interactiveGraphQuestionBuilder()
.withXRange(-6 * Math.PI, 6 * Math.PI)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,8 @@ const renderGraphElements = (props: {
return renderAbsoluteValueGraph(state, dispatch, i18n);
case "tangent":
return renderTangentGraph(state, dispatch, i18n);
case "logarithm":
return {graph: null, interactiveElementsDescription: null};
default:
throw new UnreachableCaseError(type);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
SegmentGraphState,
SinusoidGraphState,
TangentGraphState,
LogarithmGraphState,
} from "./types";
import type {PerseusGraphType} from "@khanacademy/perseus-core";

Expand Down Expand Up @@ -491,4 +492,47 @@ describe("mafsStateToInteractiveGraph", () => {
],
});
});

it("converts the state of a logarithm graph", () => {
const graph: PerseusGraphType = {
type: "logarithm",
startCoords: {
coords: [
[5, 6],
[7, 8],
],
asymptote: 3,
},
};
const state: LogarithmGraphState = {
...commonGraphState,
type: "logarithm",
coords: [
[1, 2],
[3, 4],
],
asymptote: -1,
};

const result: PerseusGraphType = mafsStateToInteractiveGraph(
state,
graph,
);

expect(result).toEqual({
type: "logarithm",
coords: [
[1, 2],
[3, 4],
],
asymptote: -1,
startCoords: {
coords: [
[5, 6],
[7, 8],
],
asymptote: 3,
},
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ export function mafsStateToInteractiveGraph(
...originalGraph,
coords: state.coords,
};
case "logarithm":
invariant(originalGraph.type === "logarithm");
return {
...originalGraph,
coords: state.coords,
asymptote: state.asymptote,
};
default:
throw new UnreachableCaseError(state);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,72 @@ describe("initializeGraphState for exponential graphs", () => {
});
});

describe("initializeGraphState for logarithm graphs", () => {
it("uses the given coords and asymptote if present", () => {
// Arrange, Act
const graph = initializeGraphState({
...baseGraphData,
graph: {
type: "logarithm",
coords: [
[-4, -3],
[-5, -7],
],
asymptote: -6,
},
});

// Assert
invariant(graph.type === "logarithm");
expect(graph.coords).toEqual([
[-4, -3],
[-5, -7],
]);
expect(graph.asymptote).toBe(-6);
});

it("uses startCoords if given and explicit coords are absent", () => {
// Arrange, Act
const graph = initializeGraphState({
...baseGraphData,
graph: {
type: "logarithm",
startCoords: {
coords: [
[1, 4],
[3, 8],
],
asymptote: 2,
},
},
});

// Assert
invariant(graph.type === "logarithm");
expect(graph.coords).toEqual([
[1, 4],
[3, 8],
]);
expect(graph.asymptote).toBe(2);
});

it("uses default coords and asymptote if neither coords nor startCoords are given", () => {
// Arrange, Act
const graph = initializeGraphState({
...baseGraphData,
graph: {type: "logarithm"},
});

// Assert
invariant(graph.type === "logarithm");
expect(graph.coords).toEqual([
[1, 1],
[5, 5],
]);
expect(graph.asymptote).toBe(0);
});
});

describe("initializeGraphState for tangent graphs", () => {
it("uses the given coords, if present", () => {
const graph = initializeGraphState({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
PerseusGraphTypeSinusoid,
PerseusGraphTypeExponential,
PerseusGraphTypeTangent,
PerseusGraphTypeLogarithm,
} from "@khanacademy/perseus-core";
import type {Interval} from "mafs";

Expand Down Expand Up @@ -149,7 +150,8 @@
case "logarithm":
return {
...shared,
type: "none",
type: graph.type,
...getLogarithmCoords(graph, range, step),
};
default:
throw new UnreachableCaseError(graph);
Expand Down Expand Up @@ -516,6 +518,37 @@
return {coords, asymptote};
}

function getLogarithmCoords(
graph: PerseusGraphTypeLogarithm,
range: [x: Interval, y: Interval],
step: [x: number, y: number],

Check warning on line 524 in packages/perseus/src/widgets/interactive-graphs/reducer/initialize-graph-state.ts

View check run for this annotation

Claude / Claude Code Review

getLogarithmCoords missing export keyword

The `getLogarithmCoords` function in `initialize-graph-state.ts` is missing the `export` keyword, unlike its counterpart `getExponentialCoords` (line 490) which is exported. The design doc (logarithm.md:307) explicitly lists exporting `getLogarithmCoords` as a PR 3 deliverable, and PR 6 (editor support) will need to import it exactly the way the editor currently imports `getExponentialCoords`.
Comment on lines +521 to +524
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The getLogarithmCoords function in initialize-graph-state.ts is missing the export keyword, unlike its counterpart getExponentialCoords (line 490) which is exported. The design doc (logarithm.md:307) explicitly lists exporting getLogarithmCoords as a PR 3 deliverable, and PR 6 (editor support) will need to import it exactly the way the editor currently imports getExponentialCoords.

Extended reasoning...

What the bug is

getLogarithmCoords is declared as function getLogarithmCoords (no export keyword) at line 521 of initialize-graph-state.ts. Its counterpart getExponentialCoords is declared export function getExponentialCoords at line 490 of the same file. The PR description explicitly states it follows the exponential pattern throughout, and the design doc at packages/perseus/src/widgets/interactive-graphs/__docs__/notes/logarithm.md line 307 lists packages/perseus/src/index.ts — Export getLogarithmCoords as a PR 3 task.

The code path that will fail

getExponentialCoords is re-exported from packages/perseus/src/index.ts (line 113) and consumed by packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts (line 6) to compute default start coordinates in the editor UI. PR 6 (editor support for logarithm) will add logarithm handling to that same util.ts, importing getLogarithmCoords the same way. Without the export keyword here and a corresponding re-export in index.ts, that import will fail to compile.

Why existing code doesn't prevent it

The function is only called internally within initializeGraphState in the same file, so the missing export produces no error today. TypeScript module boundaries are only enforced at import sites, and no import site exists yet for this function.

Addressing the refutation

The refutation argues this is not a bug because the function works for its current purpose and the export can be added in PR 6. This is a reasonable position for a strict "does this PR break anything" review, but the PR description explicitly claims to follow the exponential pattern and the design doc explicitly lists the export as a PR 3 deliverable. Omitting it means this PR does not fully deliver on its stated scope, and the gap will need to be caught before PR 6 merges — exactly the kind of thing a PR review should flag.

Impact

No runtime or compilation failure in this PR or any currently merged code. The impact is forward-compatibility: PR 6 will fail to compile when it attempts to import getLogarithmCoords from @khanacademy/perseus.

How to fix it

  1. Change function getLogarithmCoords to export function getLogarithmCoords at line 521 of initialize-graph-state.ts.
  2. Add getLogarithmCoords to the re-exports in packages/perseus/src/index.ts alongside getExponentialCoords (line 113).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too fussed if it's exported during this PR or the later PR, myself.

): {coords: [Coord, Coord]; asymptote: number} {
if (graph.coords && graph.asymptote != null) {
return {
coords: [graph.coords[0], graph.coords[1]],
asymptote: graph.asymptote,
};
}

// Default coords as normalized fractions of the graph range. After
// normalization with the default asymptote at x=0, both points will
// be to the right of the asymptote.
let defaultCoords: [Coord, Coord] = [
[0.55, 0.55],
[0.75, 0.75],
];
defaultCoords = normalizePoints(range, step, defaultCoords, true);

const coords: [Coord, Coord] = graph.startCoords
? graph.startCoords.coords
: defaultCoords;
const asymptote: number = graph.startCoords
? graph.startCoords.asymptote
: 0;

return {coords, asymptote};
}

export const getAngleCoords = (params: {
graph: PerseusGraphTypeAngle;
range: [x: Interval, y: Interval];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ export const actions = {
movePoint,
moveCenter,
},
logarithm: {
movePoint,
moveCenter,
},
absoluteValue: {
movePoint,
},
Expand Down
Loading
Loading