Skip to content
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.

Sign up for GitHub

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

Move scorers: Plotter and Sorter #2113

Merged
merged 3 commits into from
Jan 17, 2025
Merged

Move scorers: Plotter and Sorter #2113

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
226807f
plotter
handeyeco Jan 15, 2025
ac902bb
sorter
handeyeco Jan 15, 2025
806808a
Move scorer: Orderer (#2114)
handeyeco Jan 17, 2025
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
2 changes: 2 additions & 0 deletions packages/perseus-score/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ export {default as scoreIframe} from "./widgets/iframe/score-iframe";
export {default as scoreMatcher} from "./widgets/matcher/score-matcher";
export {default as scoreNumberLine} from "./widgets/number-line/score-number-line";
export {default as scoreNumericInput} from "./widgets/numeric-input/score-numeric-input";
export {default as scorePlotter} from "./widgets/plotter/score-plotter";
export {default as scoreRadio} from "./widgets/radio/score-radio";
export {default as scoreSorter} from "./widgets/sorter/score-sorter";
export {default as scoreTable} from "./widgets/table/score-table";
export {
default as scoreInputNumber,
Expand Down
2 changes: 1 addition & 1 deletion ...src/widgets/plotter/score-plotter.test.ts → ...src/widgets/plotter/score-plotter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import scorePlotter from "./score-plotter";
import type {
PerseusPlotterScoringData,
PerseusPlotterUserInput,
} from "@khanacademy/perseus-score";
} from "../../validation.types";

describe("scorePlotter", () => {
it("can be answered correctly", () => {
Expand Down
12 changes: 5 additions & 7 deletions ...seus/src/widgets/plotter/score-plotter.ts → ...core/src/widgets/plotter/score-plotter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import Util from "../../util";
import _ from "underscore";

import validatePlotter from "./validate-plotter";

import type {
PerseusScore,
PerseusPlotterScoringData,
PerseusPlotterUserInput,
} from "@khanacademy/perseus-score";

const {deepEq} = Util;
PerseusPlotterScoringData,
PerseusScore,
} from "../../validation.types";

function scorePlotter(
userInput: PerseusPlotterUserInput,
Expand All @@ -20,7 +18,7 @@ function scorePlotter(
}
return {
type: "points",
earned: deepEq(userInput, scoringData.correct) ? 1 : 0,
earned: _.isEqual(userInput, scoringData.correct) ? 1 : 0,
total: 1,
Copy link
Member

Choose a reason for hiding this comment

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

I vaguely remember we were talking about trying to get rid of underscore at some point. Was that only for functions like _.map that have native equivalents now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where native equivalents exist, I think we should switch away from underscore. For cases like this, it'd be nice if we had a single deep equality check. I think we actually want to revert this change because our "in-house" deepEq uses an approximate equals check for numeric values.

Suggested change
earned: _.isEqual(userInput, scoringData.correct) ? 1 : 0,
earned: deepEq(userInput, scoringData.correct) ? 1 : 0,
image

Copy link
Contributor Author

@handeyeco handeyeco Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's right. I need to go back and undo the change to underscore. I think what I'll do is add another PR to the top because I also renamed eq and deepEq (in a later PR) to make it clearer that they're approximate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2125

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to add another thought.

Was that only for functions like _.map that have native equivalents now?

I think we should absolutely not use jQuery/Underscore for things that have native equivalents (map, each, etc).

I also don't think we should be reinventing complex logic that's not native and provided through something like Underscore.

In this case it wasn't clear to me that these were approximate equality checks, so I didn't see the need to reinvent the wheel when we depend on Underscore and Underscore has isEqual. Now that I know that these are fairly specific helpers, I know now we should have kept them. I renamed them to approximateDeepEqual and approximateEqual to prevent people from making my mistake in the future.

message: null,
};
Expand Down
2 changes: 1 addition & 1 deletion .../widgets/plotter/validate-plotter.test.ts → .../widgets/plotter/validate-plotter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import validatePlotter from "./validate-plotter";
import type {
PerseusPlotterUserInput,
PerseusPlotterValidationData,
} from "@khanacademy/perseus-score";
} from "../../validation.types";

describe("validatePlotter", () => {
it("is invalid if the start and end are the same", () => {
Expand Down
10 changes: 4 additions & 6 deletions ...s/src/widgets/plotter/validate-plotter.ts → ...e/src/widgets/plotter/validate-plotter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import Util from "../../util";
import _ from "underscore";

import type {
ValidationResult,
PerseusPlotterUserInput,
PerseusPlotterValidationData,
} from "@khanacademy/perseus-score";

const {deepEq} = Util;
ValidationResult,
} from "../../validation.types";

/**
* Checks user input to confirm it is not the same as the starting values for the graph.
Expand All @@ -18,7 +16,7 @@ function validatePlotter(
userInput: PerseusPlotterUserInput,
validationData: PerseusPlotterValidationData,
): ValidationResult {
if (deepEq(userInput, validationData.starting)) {
if (_.isEqual(userInput, validationData.starting)) {
return {
type: "invalid",
message: null,
Expand Down
4 changes: 2 additions & 2 deletions ...s/src/widgets/sorter/score-sorter.test.ts → ...e/src/widgets/sorter/score-sorter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import scoreSorter from "./score-sorter";
import * as SorterValidator from "./validate-sorter";

import type {
PerseusSorterRubric,
PerseusSorterUserInput,
} from "@khanacademy/perseus-score";
PerseusSorterRubric,
} from "../../validation.types";

describe("scoreSorter", () => {
it("is correct when the user input values are in the order defined in the rubric", () => {
Expand Down
10 changes: 5 additions & 5 deletions ...erseus/src/widgets/sorter/score-sorter.ts → ...-score/src/widgets/sorter/score-sorter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import Util from "../../util";
import _ from "underscore";

import validateSorter from "./validate-sorter";

import type {
PerseusScore,
PerseusSorterRubric,
PerseusSorterUserInput,
} from "@khanacademy/perseus-score";
PerseusSorterRubric,
PerseusScore,
} from "../../validation.types";

function scoreSorter(
userInput: PerseusSorterUserInput,
Expand All @@ -17,7 +17,7 @@ function scoreSorter(
return validationError;
}

const correct = Util.deepEq(userInput.options, rubric.correct);
const correct = _.isEqual(userInput.options, rubric.correct);
return {
type: "points",
earned: correct ? 1 : 0,
Expand Down
2 changes: 1 addition & 1 deletion ...rc/widgets/sorter/validate-sorter.test.ts → ...rc/widgets/sorter/validate-sorter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import validateSorter from "./validate-sorter";

import type {PerseusSorterUserInput} from "@khanacademy/perseus-score";
import type {PerseusSorterUserInput} from "../../validation.types";

describe("validateSorter", () => {
it("is invalid when the user has not made any changes", () => {
Expand Down
4 changes: 2 additions & 2 deletions ...eus/src/widgets/sorter/validate-sorter.ts → ...ore/src/widgets/sorter/validate-sorter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {
ValidationResult,
PerseusSorterUserInput,
} from "@khanacademy/perseus-score";
ValidationResult,
} from "../../validation.types";

/**
* Checks user input for the sorter widget to ensure that the user has made
Expand Down
11 changes: 5 additions & 6 deletions packages/perseus/src/widgets/plotter/plotter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
/* eslint-disable react/no-unsafe */
import {KhanMath} from "@khanacademy/kmath";
import {
scorePlotter,
type PerseusPlotterScoringData,
type PerseusPlotterUserInput,
} from "@khanacademy/perseus-score";
import $ from "jquery";
import * as React from "react";
import ReactDOM from "react-dom";
Expand All @@ -13,15 +18,9 @@ import KhanColors from "../../util/colors";
import GraphUtils from "../../util/graph-utils";
import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/plotter/plotter-ai-utils";

import scorePlotter from "./score-plotter";

import type {Widget, WidgetExports, WidgetProps} from "../../types";
import type {UnsupportedWidgetPromptJSON} from "../../widget-ai-utils/unsupported-widget";
import type {PerseusPlotterWidgetOptions} from "@khanacademy/perseus-core";
import type {
PerseusPlotterScoringData,
PerseusPlotterUserInput,
} from "@khanacademy/perseus-score";

type RenderProps = PerseusPlotterWidgetOptions;

Expand Down
11 changes: 5 additions & 6 deletions packages/perseus/src/widgets/sorter/sorter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
import {linterContextDefault} from "@khanacademy/perseus-linter";
import {
scoreSorter,
type PerseusSorterRubric,
type PerseusSorterUserInput,
} from "@khanacademy/perseus-score";
import * as React from "react";

import Sortable from "../../components/sortable";
import Util from "../../util";
import {getPromptJSON as _getPromptJSON} from "../../widget-ai-utils/sorter/sorter-ai-utils";

import scoreSorter from "./score-sorter";

import type {SortableOption} from "../../components/sortable";
import type {Widget, WidgetExports, WidgetProps} from "../../types";
import type {SorterPromptJSON} from "../../widget-ai-utils/sorter/sorter-ai-utils";
import type {PerseusSorterWidgetOptions} from "@khanacademy/perseus-core";
import type {
PerseusSorterRubric,
PerseusSorterUserInput,
} from "@khanacademy/perseus-score";

const {shuffle} = Util;

Expand Down
Loading