Ensure the new Mafs-based Angle Graphs score correctly #1432

SonicScrewdriver · 2024-07-22T20:38:37Z

Summary:

The new Mafs-based Angle graphs were being marked as incorrect if the user moves the bottom point clockwise, which was not caught earlier as there was a bug in the implementation of the legacy Angle graphs that allowed users to create reflexive angles when they were not supposed to be able to.

The new Mafs-based Angle graph solves this issue, but it uncovered a bug in our scoring that fails to handle this particular state.

This ticket adds the logic necessary to reverse the coordinates (for scoring purposes) when they're clockwise and reflexive graphs are not allowed. This will be the approach used until we are able to remove/deprecate the legacy Angle graphs.

When the legacy Angle graph is removed, it's highly recommended that this logic be moved directly to the scoring function in interactive-graph.tsx, so that there's no future confusion about how the scoring is being calculated. Upgrading this graph was very difficult due to the previous logic being split across many different files, so this will save us a lot of headaches down the road. The ticket to house that work can be found here.

Issue: LEMS-2165

Test plan:

Manual testing using Flipbook
Creation of new regression tests to test all 3 states to ensure that there's no future regressions during the transition (These should be moved along with the logic during LEMS-2190)

Video:

Screen.Recording.2024-07-22.at.1.52.56.PM.mov

khan-actions-bot · 2024-07-22T20:38:55Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/sweet-seas-dance.md, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.test.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-07-22T20:40:11Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (ffb1f03) and published it to npm. You
can install it using the tag PR1432.

Example:

yarn add @khanacademy/perseus@PR1432

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1432

github-actions · 2024-07-22T20:41:21Z

Size Change: +353 B (+0.04%)

Total Size: 852 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	412 kB	+353 B (+0.09%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.8 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	272 kB
`packages/perseus-linter/dist/es/index.js`	21.6 kB
`packages/perseus/dist/es/strings.js`	3.23 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-07-22T20:46:12Z

Codecov Report

Attention: Patch coverage is 30.43478% with 16 lines in your changes missing coverage. Please review.

Project coverage is 70.52%. Comparing base (af68a9e) to head (ffb1f03).

Files	Patch %	Lines
...eractive-graphs/reducer/interactive-graph-state.ts	15.78%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1432      +/-   ##
==========================================
+ Coverage   69.93%   70.52%   +0.58%     
==========================================
  Files         508      511       +3     
  Lines      105287   105346      +59     
  Branches     5364    11405    +6041     
==========================================
+ Hits        73636    74297     +661     
+ Misses      31534    31049     -485     
+ Partials      117        0     -117

Files	Coverage Δ
packages/perseus/src/widgets/interactive-graph.tsx	`51.22% <100.00%> (+0.37%)`	⬆️
...eractive-graphs/reducer/interactive-graph-state.ts	`29.75% <15.78%> (-2.95%)`	⬇️

... and 122 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af68a9e...ffb1f03. Read the comment docs.

jeremywiebe · 2024-07-22T21:20:46Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.ts

+        ]);
+        const shouldReverseCoords = areClockwise && !state.allowReflexAngles;
+        const coords: [Coord, Coord, Coord] = shouldReverseCoords
+            ? (state.coords.slice().reverse() as [Coord, Coord, Coord])


I think you can get away without the as if you use:

Suggested change

? (state.coords.slice().reverse() as [Coord, Coord, Coord])

? ([...state.coords].reverse()

Sadly no luck here :(

Reverse returns a generic array (Coord[]), and we're looking specifically for [Coord, Coord, Coord], so typescript becomes unhappy.

Ah right! It's reverse() that's part of the problem!

jeremywiebe · 2024-07-22T21:21:11Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.ts

        return {
            ...initialGraph,
-            coords: state.coords,
+            coords: coords,


nit

Suggested change

coords: coords,

coords,

jeremywiebe · 2024-07-22T21:22:10Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.ts

+        // we don't allow reflex angles. This is because the angle is largely defined
+        // by the order of the points in the coords array, and we want to maintain
+        // the same angle scoring with the legacy graph (which had a bug).
+        // (LEMS-2190): When we remove the legacy graph, move this logic to the scoring function.


Thanks for leaving such a great, explanatory comment. I was thinking about removing the old code and how we'd know about LEMS-2190 at that time. What do you think of also adding a reference to LEMS-2190 at the top of the legacy interactive-graph file? Or near some code that we know we'll be touching when we delete it.

jeremywiebe · 2024-07-22T21:30:06Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-state.test.ts

+    // is reflexive in a clockwise direction in order to temporarily maintain the same angle scoring with the
+    // legacy graph. This logic (& the tests) should be moved to the scoring function after removing the legacy graph.
+    it("returns reversed coordinates if the angle graph is reflexive when not allowed", () => {
+        const state: InteractiveGraphState = {


Thinking about the builder work that Ben/Nisha have done... in these three tests, the only bits that change are the coords and allowReflexAngles right?

I wonder if it's worth building a InteractiveGraphState like this:

const defaultState: InteractiveGraphState = { type: "angle", coords: [ [-5, 0], [0, 0], [5, 5], ], hasBeenInteractedWith: true, range: [ [-10, 10], [-10, 10], ], snapStep: [1, 1], showAngles: true, allowReflexAngles: false, };

And then in the tests:

const state = { ...defaultState, coords: [[], [], []], allowReflexAngles: true, // or false }

That would distill the data to only the bits that area essential to the test. 🤔

"All these pieces, black and swift Are moving through themselves"

@nishasy

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] ### Minor Changes - [#1468](#1468) [`af68a9e08`](af68a9e) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for sinusoid graphs - [#1469](#1469) [`6e1ec850c`](6e1ec85) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for quadratic graphs ### Patch Changes - [#1470](#1470) [`942b0a9a5`](942b0a9) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Locked Figures] Remove m2 flag from the code - [#1465](#1465) [`94ad04fee`](94ad04f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add separate flags for graph types - [#1432](#1432) [`ed6737025`](ed67370) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Bug fix to ensure that new angle graphs are scored correctly. ## @khanacademy/[email protected] ### Minor Changes - [#1468](#1468) [`af68a9e08`](af68a9e) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for sinusoid graphs - [#1469](#1469) [`6e1ec850c`](6e1ec85) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for quadratic graphs ### Patch Changes - [#1470](#1470) [`942b0a9a5`](942b0a9) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Locked Figures] Remove m2 flag from the code - [#1465](#1465) [`94ad04fee`](94ad04f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add separate flags for graph types - Updated dependencies \[[`af68a9e08`](af68a9e), [`942b0a9a5`](942b0a9), [`6e1ec850c`](6e1ec85), [`94ad04fee`](94ad04f), [`ed6737025`](ed67370)]: - @khanacademy/[email protected]

SonicScrewdriver self-assigned this Jul 22, 2024

khan-actions-bot requested a review from a team July 22, 2024 20:38

SonicScrewdriver changed the title ~~Added the changeset~~ Ensure the new Mafs-based Angle Graphs score correctly Jul 22, 2024

jeremywiebe approved these changes Jul 22, 2024

View reviewed changes

SonicScrewdriver force-pushed the reflexive-scoring branch from a41768b to 80d8769 Compare July 22, 2024 23:10

SonicScrewdriver added 4 commits July 31, 2024 14:31

Fix for angle graph scoring issue

eaa8230

Added the changeset

bf10931

"All these pieces, black and swift Are moving through themselves"

slight comment update

2e26d6f

Addressing PR Feedback

ffb1f03

benchristel force-pushed the reflexive-scoring branch from 80d8769 to ffb1f03 Compare July 31, 2024 21:32

benchristel merged commit ed67370 into main Jul 31, 2024
10 of 11 checks passed

benchristel deleted the reflexive-scoring branch July 31, 2024 22:02

khan-actions-bot mentioned this pull request Jul 31, 2024

Version Packages #1467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the new Mafs-based Angle Graphs score correctly #1432

Ensure the new Mafs-based Angle Graphs score correctly #1432

SonicScrewdriver commented Jul 22, 2024 •

edited

Loading

khan-actions-bot commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

jeremywiebe Jul 22, 2024

SonicScrewdriver Jul 22, 2024

jeremywiebe Jul 22, 2024

jeremywiebe Jul 22, 2024

jeremywiebe Jul 22, 2024

jeremywiebe Jul 22, 2024

	? (state.coords.slice().reverse() as [Coord, Coord, Coord])
	? ([...state.coords].reverse()

Ensure the new Mafs-based Angle Graphs score correctly #1432

Ensure the new Mafs-based Angle Graphs score correctly #1432

Conversation

SonicScrewdriver commented Jul 22, 2024 • edited Loading

Summary:

Test plan:

Video:

khan-actions-bot commented Jul 22, 2024 • edited Loading

Gerald

github-actions bot commented Jul 22, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Jul 22, 2024 • edited Loading

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

jeremywiebe Jul 22, 2024

Choose a reason for hiding this comment

SonicScrewdriver Jul 22, 2024

Choose a reason for hiding this comment

jeremywiebe Jul 22, 2024

Choose a reason for hiding this comment

jeremywiebe Jul 22, 2024

Choose a reason for hiding this comment

jeremywiebe Jul 22, 2024

Choose a reason for hiding this comment

jeremywiebe Jul 22, 2024

Choose a reason for hiding this comment

SonicScrewdriver commented Jul 22, 2024 •

edited

Loading

khan-actions-bot commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024 •

edited

Loading

github-actions bot commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading