-
Notifications
You must be signed in to change notification settings - Fork 15
fix(studio-app): compact loss chart points instead of tail-slicing (#215) #230
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
ec7882d
c862527
c7134bf
94cf5e3
7175375
3f22473
20de904
3dab35e
46e674b
37dfceb
6714fe2
724217e
374341d
b1c1e55
43a1dc3
5207348
f3ca4b9
e2fd994
a0497b0
6fc02d3
6652d36
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,128 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { compactLossPoints } from "./lossDownsample"; | ||
| import type { LossPoint } from "../components/jobs/LossChart"; | ||
|
|
||
| function point( | ||
| step: number, | ||
| loss: number | null, | ||
| evalLoss?: number | null, | ||
| ): LossPoint { | ||
| return evalLoss === undefined ? { step, loss } : { step, loss, evalLoss }; | ||
| } | ||
|
|
||
| function isSortedByStep(points: LossPoint[]): boolean { | ||
| for (let i = 1; i < points.length; i++) { | ||
| if (points[i].step < points[i - 1].step) return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| describe("compactLossPoints", () => { | ||
| it("returns the input untouched when already at or under targetSize", () => { | ||
| const points = [point(1, 0.5), point(2, 0.4), point(3, 0.3)]; | ||
| expect(compactLossPoints(points, 3)).toBe(points); | ||
| expect(compactLossPoints(points, 10)).toBe(points); | ||
| }); | ||
|
|
||
| it("returns an empty array for empty input", () => { | ||
| expect(compactLossPoints([], 10)).toEqual([]); | ||
| }); | ||
|
|
||
| it("returns an empty array when targetSize is 0 or negative", () => { | ||
| const points = [point(1, 0.5), point(2, 0.4)]; | ||
| expect(compactLossPoints(points, 0)).toEqual([]); | ||
| expect(compactLossPoints(points, -5)).toEqual([]); | ||
| }); | ||
|
|
||
| it("returns only the last point when targetSize is 1", () => { | ||
| const points = [point(1, 0.9), point(2, 0.5), point(3, 0.1)]; | ||
| expect(compactLossPoints(points, 1)).toEqual([point(3, 0.1)]); | ||
| }); | ||
|
|
||
| it("always preserves the first and last point", () => { | ||
| const points = Array.from({ length: 100 }, (_, i) => point(i, 1 / (i + 1))); | ||
| const result = compactLossPoints(points, 10); | ||
| expect(result[0]).toEqual(points[0]); | ||
| expect(result.at(-1)).toEqual(points.at(-1)); | ||
| }); | ||
|
|
||
| it("preserves every point with a finite evalLoss", () => { | ||
| const points = Array.from({ length: 200 }, (_, i) => | ||
| i % 37 === 0 ? point(i, 1, 0.8) : point(i, 1), | ||
| ); | ||
| const evalSteps = points | ||
| .filter((p) => typeof p.evalLoss === "number") | ||
| .map((p) => p.step); | ||
| const result = compactLossPoints(points, 20); | ||
| const resultEvalSteps = result | ||
| .filter((p) => typeof p.evalLoss === "number") | ||
| .map((p) => p.step); | ||
| expect(resultEvalSteps).toEqual(evalSteps); | ||
| }); | ||
|
|
||
| it("preserves local minima and maxima of the loss series", () => { | ||
| // A single sharp spike in the middle of an otherwise flat series. | ||
| const points = Array.from({ length: 50 }, (_, i) => | ||
| point(i, i === 25 ? 99 : 1), | ||
| ); | ||
| const result = compactLossPoints(points, 10); | ||
| expect(result.some((p) => p.step === 25 && p.loss === 99)).toBe(true); | ||
| }); | ||
|
|
||
| it("does not flood the must-keep set on a flat/constant loss series", () => { | ||
| // Ties on both sides shouldn't count as an extremum; a constant | ||
| // series should compact down to essentially first/last plus even | ||
| // sampling, not near-full retention. | ||
| const points = Array.from({ length: 1000 }, (_, i) => point(i, 1)); | ||
| const result = compactLossPoints(points, 50); | ||
| expect(result.length).toBeLessThanOrEqual(50); | ||
| }); | ||
|
|
||
| it("keeps output sorted by step (subsequence of input)", () => { | ||
| const points = Array.from({ length: 5000 }, (_, i) => | ||
| point(i, Math.sin(i / 50)), | ||
| ); | ||
| const result = compactLossPoints(points, 500); | ||
| expect(isSortedByStep(result)).toBe(true); | ||
| }); | ||
|
|
||
| it("never exceeds targetSize even when must-keep points alone exceed it", () => { | ||
| // Every point carries a finite evalLoss, so the must-keep set is | ||
| // the entire array; the function must still respect the cap by | ||
| // evenly sampling down from the must-keep set itself. | ||
| const points = Array.from({ length: 300 }, (_, i) => point(i, 1, i * 0.01)); | ||
| const result = compactLossPoints(points, 50); | ||
| expect(result.length).toBeLessThanOrEqual(50); | ||
| expect(result[0]).toEqual(points[0]); | ||
| expect(result.at(-1)).toEqual(points.at(-1)); | ||
| }); | ||
|
|
||
| it("simulates a long training run: compacting to half the cap repeatedly stays bounded and keeps the run's start visible", () => { | ||
| // Mirrors how JobDetail.tsx would call this: compact to | ||
| // MAX_LOSS_POINTS/2 each time the cap is hit, then keep appending. | ||
| const MAX = 2000; | ||
| let points: LossPoint[] = []; | ||
| for (let step = 0; step < 50_000; step++) { | ||
| points.push(point(step, Math.exp(-step / 10_000))); | ||
| if (points.length > MAX) { | ||
| points = compactLossPoints(points, MAX / 2); | ||
| } | ||
| } | ||
| expect(points.length).toBeLessThanOrEqual(MAX); | ||
| // The very first step of the run must still be present somewhere, | ||
| // which is exactly the bug #215 reports against tail-slicing. | ||
| expect(points.some((p) => p.step === 0)).toBe(true); | ||
| expect(isSortedByStep(points)).toBe(true); | ||
| }); | ||
|
|
||
| it("treats loss values separated by null-loss frames as still adjacent for extrema detection", () => { | ||
| const points = [ | ||
| point(0, 1), | ||
| point(1, null, 0.5), // no loss, only evalLoss | ||
| point(2, 5), // local max relative to steps 0 and 3 | ||
| point(3, 1), | ||
| ]; | ||
| const result = compactLossPoints(points, 3); | ||
| expect(result.some((p) => p.step === 2 && p.loss === 5)).toBe(true); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import type { LossPoint } from "../components/jobs/LossChart"; | ||
|
|
||
| /** | ||
| * Compacts `points` down to at most `targetSize` representatives while | ||
| * preserving: | ||
| * - the first and last point (run boundaries) | ||
| * - every point carrying a finite `evalLoss` (that series is sparse | ||
| * relative to `loss`, so keeping all of it is cheap) | ||
| * - local minima/maxima of the `loss` series (so visible spikes | ||
| * survive compaction) | ||
| * Remaining budget is filled with evenly-spaced points from what's | ||
| * left, so the overall shape of the run stays visible at coarser | ||
| * resolution. | ||
| * | ||
| * Output is always a subsequence of the input in original order, so | ||
| * callers relying on the array staying sorted by `step` (e.g. | ||
| * `LossChart`'s binary-search tooltip) are unaffected. | ||
| */ | ||
| export function compactLossPoints( | ||
| points: LossPoint[], | ||
| targetSize: number, | ||
| ): LossPoint[] { | ||
| if (targetSize < 1) return []; | ||
| if (points.length <= targetSize) return points; | ||
| if (targetSize === 1) { | ||
| const last = points.at(-1); | ||
| return last ? [last] : []; | ||
| } | ||
|
|
||
| const mustKeep = new Set<number>([0, points.length - 1]); | ||
|
|
||
| for (const [i, point] of points.entries()) { | ||
| const e = point.evalLoss; | ||
| if (typeof e === "number" && Number.isFinite(e)) mustKeep.add(i); | ||
| } | ||
|
|
||
| // Local minima/maxima of the `loss` series, compared across the | ||
| // filtered subsequence of non-null loss values so a run of null-loss | ||
| // frames between two real values doesn't block extrema detection. | ||
| const lossIndices: number[] = []; | ||
| for (const [i, point] of points.entries()) { | ||
| if (point.loss !== null) lossIndices.push(i); | ||
| } | ||
| for (let k = 1; k < lossIndices.length - 1; k++) { | ||
| const prev = points[lossIndices[k - 1]].loss; | ||
| const cur = points[lossIndices[k]].loss; | ||
| const next = points[lossIndices[k + 1]].loss; | ||
| // `lossIndices` only holds indices where `loss !== null`, so this | ||
| // is unreachable; the guard just keeps the comparison below | ||
| // type-safe without a non-null assertion. | ||
| if (prev === null || cur === null || next === null) continue; | ||
| // Requiring strictness on at least one side (rather than allowing | ||
| // `cur <= prev && cur <= next` alone) keeps flat/constant loss | ||
| // runs from flagging almost every point as a "tied" extremum, | ||
| // which would otherwise flood `mustKeep` and crowd out genuine | ||
| // spikes once the must-keep set exceeds `targetSize` and falls | ||
| // back to even-sampling. | ||
| const isMax = cur >= prev && cur >= next && (cur > prev || cur > next); | ||
| const isMin = cur <= prev && cur <= next && (cur < prev || cur < next); | ||
| if (isMax || isMin) { | ||
| mustKeep.add(lossIndices[k]); | ||
| } | ||
| } | ||
|
|
||
| const mustKeepSorted = [...mustKeep].toSorted((a, b) => a - b); | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
coderabbitai[bot] marked this conversation as resolved.
Outdated
sentry[bot] marked this conversation as resolved.
Outdated
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.
When an older supported browser receives the 2,001st loss point, this newly reached call to Useful? React with 👍 / 👎.
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| let selected: number[]; | ||
| if (mustKeepSorted.length >= targetSize) { | ||
| // Too many "must keep" points to fit the budget: evenly sample | ||
| // down from the must-keep set itself. `evenSample` always retains | ||
| // its own first/last entry, so run boundaries survive either way. | ||
| selected = evenSample(mustKeepSorted, targetSize); | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
coderabbitai[bot] marked this conversation as resolved.
Outdated
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.
For highly oscillatory loss data where the extrema set itself exceeds the target, evenly sampling these indices can erase the oscillation and invent a broad trend. With 2,001 points alternating between 0 and 1, the rounded 2,000/999 stride selects long runs of one parity, so the retained chart is approximately 0 for the first quarter, 1 for the middle half, and 0 for the final quarter instead of showing the repeated peaks. Use bucketed min/max selection or another scheme that retains both sides of high-frequency oscillations when falling back under the cap. Useful? React with 👍 / 👎.
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
|
||
| } else { | ||
| const remainingBudget = targetSize - mustKeepSorted.length; | ||
| const notKept: number[] = []; | ||
| for (const [i] of points.entries()) { | ||
| if (!mustKeep.has(i)) notKept.push(i); | ||
| } | ||
| const filler = evenSample(notKept, remainingBudget); | ||
| selected = [...mustKeepSorted, ...filler].toSorted((a, b) => a - b); | ||
| } | ||
|
|
||
| return selected.map((i) => points[i]); | ||
| } | ||
|
|
||
| /** | ||
| * Picks `count` evenly-spaced entries from `indices` (already sorted | ||
| * ascending), always keeping the first and last entry. Callers only | ||
| * invoke this when `indices.length >= count`. | ||
| */ | ||
| function evenSample(indices: number[], count: number): number[] { | ||
| if (count <= 0) return []; | ||
| if (count >= indices.length) return indices; | ||
| if (count === 1) { | ||
| const last = indices.at(-1); | ||
| return last === undefined ? [] : [last]; | ||
| } | ||
|
|
||
| const lastIdx = indices.length - 1; | ||
| const result: number[] = []; | ||
| for (let i = 0; i < count; i++) { | ||
| const pos = Math.round((i * lastIdx) / (count - 1)); | ||
| result.push(indices[pos]); | ||
| } | ||
| // Rounding can collide on nearby positions when `count` is close to | ||
| // `indices.length`; dedupe while preserving ascending order. | ||
| return [...new Set(result)]; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import { | |
| NO_VALUE_PLACEHOLDER, | ||
| truncateMiddle, | ||
| } from "../lib/format"; | ||
| import { compactLossPoints } from "../lib/lossDownsample"; | ||
|
|
||
| const MAX_LOSS_POINTS = 2000; | ||
|
|
||
|
|
@@ -219,7 +220,12 @@ export function JobDetail({ jobId }: { jobId: string }) { | |
| if (safeLoss === null && safeEvalLoss === null) return; | ||
| // Cap retained points so long/high-step runs don't grow without | ||
| // bound and slow LossChart re-renders. 2000 is well above the | ||
| // chart's visual resolution at any reasonable width. | ||
| // chart's visual resolution at any reasonable width. Once the | ||
| // cap is hit, compact down to half via `compactLossPoints` | ||
| // (stride-doubling) rather than tail-slicing, so the start of | ||
| // long runs stays visible instead of being silently dropped | ||
| // (see #215). Subsequent frames keep appending until the cap | ||
| // is hit again, at which point we compact again. | ||
| setPoints((prev) => { | ||
| const next = [ | ||
| ...prev, | ||
|
|
@@ -230,7 +236,7 @@ export function JobDetail({ jobId }: { jobId: string }) { | |
| }, | ||
| ]; | ||
| return next.length > MAX_LOSS_POINTS | ||
| ? next.slice(next.length - MAX_LOSS_POINTS) | ||
| ? compactLossPoints(next, MAX_LOSS_POINTS / 2) | ||
|
Contributor
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 changes Studio's loss-chart retention behavior without the required corresponding English and Japanese documentation, leaving users and maintainers without a documented description of the new compaction semantics and limits. Context Used: AGENTS.md (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/studio-app/src/pages/JobDetail.tsx
Line: 236-239
Comment:
**Paired behavior docs are missing**
This changes Studio's loss-chart retention behavior without the required corresponding English and Japanese documentation, leaving users and maintainers without a documented description of the new compaction semantics and limits.
**Context Used:** AGENTS.md ([source](https://github.com/arkorlab/arkor/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! 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.
Once a run exceeds 2,000 points, this replaces the array that Useful? React with 👍 / 👎.
cubic-dev-ai[bot] marked this conversation as resolved.
Comment on lines
334
to
+335
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.
When the stream re-emits a step or sends complementary loss/eval frames for the same step, Useful? React with 👍 / 👎. |
||
| : next; | ||
| }); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.