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

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
+376 −117
Open

Refactor LabelImage to separate out answers from userInput into scoringData #1965

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
ee7ad47
Rename MarkerType to MarkerAnswers
Myranae Dec 6, 2024
f36f767
Rename rubric to scoringData
Myranae Dec 6, 2024
8a3db65
Remove duplicate type
Myranae Dec 6, 2024
4164ebc
Separate answers out to scoringData
Myranae Dec 6, 2024
7fc9de8
Combine marker data to use with scoreMarker
Myranae Dec 6, 2024
e694b90
Update tests for scoring
Myranae Dec 6, 2024
c2d75b2
Merge branch 'main' into tb/LEMS-2440/label-image-review-validation-w…
Myranae Dec 6, 2024
6ae620f
Add changeset
Myranae Dec 6, 2024
e6e2d26
Update changeset
Myranae Dec 6, 2024
95467c8
Update type
Myranae Dec 6, 2024
d6870b8
Fix a type change that was incorrect
Myranae Dec 6, 2024
b0d5329
Return order to original order
Myranae Dec 6, 2024
96af0c4
Update type to not require answers
Myranae Dec 6, 2024
883f9bc
Update comment to look into how scoreMarker is used
Myranae Dec 6, 2024
c6f7d86
Update changeset
Myranae Dec 10, 2024
1579702
Update types based on PR feedback
Myranae Dec 10, 2024
35c610b
Update scoreMarker to keep selected and answers separate
Myranae Dec 10, 2024
b7955c2
Add test confirming userInput does not contain answers
Myranae Dec 10, 2024
7d8b0fe
Update getUserInput to remove answers
Myranae Dec 10, 2024
fb52876
Revert changes to perseus-types.ts
Myranae Dec 10, 2024
aae57e0
Fixing linting
Myranae Dec 10, 2024
52e7d26
Revert all changes to widget types
Myranae Dec 12, 2024
5fe1979
Expand types and remove old widget types
Myranae Dec 12, 2024
64e7d4b
Change types from old widget types to validation types
Myranae Dec 12, 2024
c83fef5
Change from old widget types to widget options
Myranae Dec 12, 2024
0dd5f68
Refer to props type not validation type
Myranae Dec 12, 2024
b0ccaf3
Remove reference to ScoringData type
Myranae Dec 12, 2024
e5b0d26
Update getUserInput to build wanted object
Myranae Dec 12, 2024
78a37df
Removed parameter and updated comment
Myranae Dec 12, 2024
2925dc8
Merge branch 'main' into tb/LEMS-2440/label-image-review-validation-w…
Myranae Dec 12, 2024
3569bc0
Remove unnecessary export
Myranae Dec 12, 2024
b7b84ff
Update test to reflect new type
Myranae Dec 12, 2024
a433df6
Add new tests
Myranae Dec 12, 2024
aa6842e
Update snapshot
Myranae Dec 12, 2024
188b6f5
Fix test syntax
Myranae Dec 12, 2024
0dc8936
Update tests to not need a snapshot
Myranae Dec 12, 2024
42a5dc3
Add a todo
Myranae Dec 12, 2024
9829693
Revert unneeded const removal
Myranae Dec 12, 2024
a342a49
Make test name more generic
Myranae Dec 13, 2024
415f32a
Update tests to reflect updated type
Myranae Dec 13, 2024
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
6 changes: 6 additions & 0 deletions .changeset/many-penguins-hug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": minor
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you changed PerseusTypes in a way that's not backwards compatible, I'd call this a major/breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to major. Thanks for pointing this out!

"@khanacademy/perseus-editor": patch
---

Refactor the LabelImage widget to separate out answers from userInput into scoringData
4 changes: 2 additions & 2 deletions packages/perseus-editor/src/widgets/__stories__/label-image-editor.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import * as React from "react";

import LabelImageEditor from "../label-image-editor";

import type {MarkerType} from "@khanacademy/perseus";
import type {MarkerAnswers} from "@khanacademy/perseus";

type StoryArgs = Record<any, any>;

Expand All @@ -29,7 +29,7 @@ type State = {
imageUrl: string;
imageWidth: number;
imageHeight: number;
markers: ReadonlyArray<MarkerType>;
markers: ReadonlyArray<MarkerAnswers>;
};

class WithState extends React.Component<Empty, State> {
Expand Down
8 changes: 4 additions & 4 deletions packages/perseus-editor/src/widgets/label-image-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import Behavior from "./label-image/behavior";
import QuestionMarkers from "./label-image/question-markers";
import SelectImage from "./label-image/select-image";

import type {MarkerType} from "@khanacademy/perseus";
import type {MarkerAnswers} from "@khanacademy/perseus";

type Props = {
// List of answer choices to label question image with.
Expand All @@ -28,7 +28,7 @@ type Props = {
imageWidth: number;
imageHeight: number;
// The list of label markers on the question image.
markers: ReadonlyArray<MarkerType>;
markers: ReadonlyArray<MarkerAnswers>;
// Whether multiple answer choices may be selected for markers.
multipleAnswers: boolean;
// Whether to hide answer choices from user instructions.
Expand Down Expand Up @@ -176,8 +176,8 @@ class LabelImageEditor extends React.Component<Props> {
this.props.onChange({choices});
};

handleMarkersChange: (markers: ReadonlyArray<MarkerType>) => void = (
markers: ReadonlyArray<MarkerType>,
handleMarkersChange: (markers: ReadonlyArray<MarkerAnswers>) => void = (
markers: ReadonlyArray<MarkerAnswers>,
) => {
this.props.onChange({markers});
};
Expand Down
4 changes: 2 additions & 2 deletions packages/perseus-editor/src/widgets/label-image/__stories__/question-markers.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as React from "react";

import QuestionMarkers from "../question-markers";

import type {MarkerType} from "@khanacademy/perseus";
import type {MarkerAnswers} from "@khanacademy/perseus";

type StoryArgs = Record<any, any>;

Expand Down Expand Up @@ -31,7 +31,7 @@ const Wrapper = (props) => (
class WithState extends React.Component<
Record<any, any>,
{
markers: ReadonlyArray<MarkerType>;
markers: ReadonlyArray<MarkerAnswers>;
}
> {
state = {
Expand Down
6 changes: 3 additions & 3 deletions packages/perseus-editor/src/widgets/label-image/marker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import Option, {OptionGroup} from "../../components/dropdown-option";
import FormWrappedTextField from "../../components/form-wrapped-text-field";
import {gray17, gray85, gray98} from "../../styles/global-colors";

import type {MarkerType} from "@khanacademy/perseus";
import type {MarkerAnswers} from "@khanacademy/perseus";

type Props = MarkerType & {
type Props = MarkerAnswers & {
// The list of possible answer choices.
choices: ReadonlyArray<string>;
// Callback for when any of the marker props are changed.
onChange: (marker: MarkerType) => void;
onChange: (marker: MarkerAnswers) => void;
// Callback to remove marker from the question image.
onRemove: () => void;
};
Expand Down
6 changes: 3 additions & 3 deletions packages/perseus-editor/src/widgets/label-image/question-markers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {gray17, gray68} from "../../styles/global-colors";

import Marker from "./marker";

import type {MarkerType} from "@khanacademy/perseus";
import type {MarkerAnswers} from "@khanacademy/perseus";

type Props = {
// The list of possible answers in a specific order.
Expand All @@ -21,9 +21,9 @@ type Props = {
imageWidth: number;
imageHeight: number;
// The list of markers placed on the question image.
markers: ReadonlyArray<MarkerType>;
markers: ReadonlyArray<MarkerAnswers>;
// Callback for when any of markers change.
onChange: (markers: ReadonlyArray<MarkerType>) => void;
onChange: (markers: ReadonlyArray<MarkerAnswers>) => void;
};

export default class QuestionMarkers extends React.Component<Props> {
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ export type {
} from "./perseus-types";
export type {UserInputMap} from "./validation.types";
export type {Coord} from "./interactive2/types";
export type {MarkerType} from "./widgets/label-image/types";
export type {MarkerAnswers} from "./widgets/label-image/types";

/**
* Multi-items
Expand Down
14 changes: 2 additions & 12 deletions packages/perseus/src/perseus-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// TODO(FEI-4011): Use types generated by https://github.com/jaredly/generate-perseus-flowtypes

import type {Coord} from "./interactive2/types";
import type {MarkerAnswers} from "./widgets/label-image/types";
import type {Interval, vec} from "mafs";

// Range is replaced within this file with Interval, but it is used elsewhere
Expand Down Expand Up @@ -1050,7 +1051,7 @@ export type PerseusLabelImageWidgetOptions = {
// The width of the image
imageWidth: number;
// A list of markers to display on the image
markers: ReadonlyArray<PerseusLabelImageMarker>;
markers: ReadonlyArray<MarkerAnswers>;
// Do not display answer choices in instructions
hideChoicesFromInstructions: boolean;
// Allow multiple answers per marker
Expand All @@ -1059,17 +1060,6 @@ export type PerseusLabelImageWidgetOptions = {
static: boolean;
};

export type PerseusLabelImageMarker = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming you got rid of this type because it was a duplicate of what's in the widget? Could you instead, make this type the source of truth? perseus-types.ts is the source of truth for the data that we serialize out of Perseus and I've been working to keep all non-trivial type definitions for widget options here instead of in the widget folders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Further, because TypeScript types match if they are the same shape, I was thinking about ScoringData and ValidationData for each widget as a matching subset of the widget options in this file.

Copy link
Contributor Author

@Myranae Myranae Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean use the types in this file (perseus-types.ts) to define the ScoringData and ValidationData types (in validation.types.ts)? Also, with Matthew's suggestion, I ended up modifying the types in the label-image/types.ts file. I'll review what I did and what was here to see if I can make them work together still. Or maybe I can just leave this file alone 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeremywiebe is particular when it comes to changing perseus-types.ts, I would have him sign off first.

// A list of correct answers for this marker. Often only one but can have multiple
answers: ReadonlyArray<string>;
// Translatable Text; The text to show for the marker. Not displayed directly to the user
label: string;
// X Coordiate location of the marker on the image
x: number;
// Y Coordinate location of the marker on the image
y: number;
};

export type PerseusMatcherWidgetOptions = {
// Translatable Text; Labels to adorn the headings for the columns. Only 2 values [left, right]. e.g. ["Concepts", "Things"]
labels: ReadonlyArray<string>;
Expand Down
15 changes: 9 additions & 6 deletions packages/perseus/src/validation.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ import type {
PerseusRadioChoice,
PerseusGraphCorrectType,
} from "./perseus-types";
import type {InteractiveMarkerType} from "./widgets/label-image/types";
import type {
InteractiveMarkerType,
MarkerAnswers,
} from "./widgets/label-image/types";
import type {Relationship} from "./widgets/number-line/number-line";

export type UserInputStatus = "correct" | "incorrect" | "incomplete";
Expand Down Expand Up @@ -138,12 +141,12 @@ export type PerseusInteractiveGraphRubric = {

export type PerseusInteractiveGraphUserInput = PerseusGraphType;

/* TODO(LEMS-2440): Should be removed or refactored. Grading info may need
to be moved to the rubric from userInput. */
export type PerseusLabelImageRubric = Empty;
export type PerseusLabelImageScoringData = {
markers: ReadonlyArray<MarkerAnswers>;
};

export type PerseusLabelImageUserInput = {
markers: ReadonlyArray<InteractiveMarkerType>;
markers: ReadonlyArray<Omit<InteractiveMarkerType, "answers">>;
Copy link
Contributor Author

@Myranae Myranae Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These are the main changes. Changed PerseusLabelImageUserInput so it no longer includes answers in the array. Then I moved the parts of InteractiveMarkerType needed for scoring to PerseusLabelImageScoringData.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little higher-risk, but I think we should make this test case pass:

    // label-image.test.ts

    describe("getUserInput", () => {
        it("doesn't include answer in getUserInput", async () => {
            // render component
            const {renderer} = renderQuestion(textQuestion);

            const userInput = renderer.getUserInputMap();

            expect(userInput).toEqual({
                "label-image 1": {
                    markers: [
                        {
                            label: "The fourth unlabeled bar line.",
                            x: 25,
                            y: 17.7,
                        },
                        {
                            label: "The third unlabeled bar line.",
                            x: 25,
                            y: 35.3,
                        },
                        {
                            label: "The second unlabeled bar line.",
                            x: 25,
                            y: 53,
                        },
                        {
                            label: "The first unlabeled bar line.",
                            x: 25,
                            y: 70.3,
                        },
                    ],
                },
            });
        });
    });

I think there should be another set of tests too:

  1. Render a LabelImage widget using ItemData that has no answers (to simulate how it will be on the FE)
  2. Answer the LabelImage (both wrong and right)
  3. Use getUserInput on the Renderer to get the user input
  4. Use scorePerseusItem to score the user input

More than anything, I'm just surprised that this ticket ended up being more of a types change whereas I was thinking it would end up being more of a logic change. Not saying you're wrong, it just makes me worried we're missing something.

};

export type PerseusMatcherRubric = PerseusMatcherWidgetOptions;
Expand Down Expand Up @@ -247,7 +250,7 @@ export type Rubric =
| PerseusIFrameRubric
| PerseusInputNumberRubric
| PerseusInteractiveGraphRubric
| PerseusLabelImageRubric
| PerseusLabelImageScoringData
| PerseusMatcherRubric
| PerseusMatrixRubric
| PerseusNumberLineScoringData
Expand Down
18 changes: 11 additions & 7 deletions packages/perseus/src/widgets/label-image/label-image.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import type {ChangeableProps} from "../../mixins/changeable";
import type {PerseusLabelImageWidgetOptions} from "../../perseus-types";
import type {APIOptions, Widget, WidgetExports} from "../../types";
import type {
PerseusLabelImageRubric,
PerseusLabelImageScoringData,
PerseusLabelImageUserInput,
} from "../../validation.types";
import type {LabelImagePromptJSON} from "../../widget-ai-utils/label-image/label-image-ai-utils";
Expand Down Expand Up @@ -72,8 +72,6 @@ type Point = {

type LabelImageProps = ChangeableProps &
DependencyProps &
// TODO: there's some weirdness in our types between
// PerseusLabelImageMarker and InteractiveMarkerType
Comment on lines -75 to -76
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was able to resolve this by removing PerseusLabelImageMarker

Omit<PerseusLabelImageWidgetOptions, "markers"> & {
apiOptions: APIOptions;
// The list of label markers on the question image.
Expand Down Expand Up @@ -194,7 +192,7 @@ export class LabelImage
*/
static navigateToMarkerIndex(
navigateDirection: Direction,
markers: ReadonlyArray<InteractiveMarkerType>,
markers: PerseusLabelImageUserInput["markers"],
thisIndex: number,
): number {
const thisMarker = markers[thisIndex];
Expand Down Expand Up @@ -318,8 +316,11 @@ export class LabelImage
return _getPromptJSON(this.props, this.getUserInput());
}

// TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove rubric parameter.
showRationalesForCurrentlySelectedChoices(rubric: PerseusLabelImageRubric) {
// TODO(LEMS-2544): Investigate impact on scoring; possibly pull out &/or remove scoringData parameter.
// Also consider how scoreMarker is being called as it seems to require the marker.answers property.
showRationalesForCurrentlySelectedChoices(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of a problem as it requires scoringData on the client-side it seems like. Since this has a TODO associated with it, I left it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a known issue: https://khanacademy.atlassian.net/browse/LEMS-2544

Luckily AX won't use rationales so we can punt on this for now.

scoringData: PerseusLabelImageScoringData,
) {
const {markers} = this.props;
const {onChange} = this.props;

Expand All @@ -342,7 +343,10 @@ export class LabelImage
onChange({markers: updatedMarkers}, null, true);
}

handleMarkerChange(index: number, marker: InteractiveMarkerType) {
handleMarkerChange(
index: number,
marker: PerseusLabelImageUserInput["markers"][number],
) {
const {markers, onChange} = this.props;

// Replace marker with a changed version at the specified index.
Expand Down
Loading
Loading