Keep the frame minimum when a gap marker shares the frame

The chart data modules push a null y value to break the line where an
entity was unavailable. downSampleLineData read it with Number(), and
Number(null) is 0, which is not NaN, so the isNaN guard did not fire.
The marker then competed as a real value of 0 and won its frame's
minimum slot whenever the readings were positive, discarding the
frame's actual minimum and widening the rendered gap.

Keep markers out of the min/max comparisons entirely and hold at most
one per frame in its own slot. It is emitted, after the frame's values,
only when no kept value follows it: a marker followed by a value in its
own frame is a gap that closed within one frame, which is about one
device pixel wide and too narrow to show. That check runs per frame at
emit time, so the per-point path stays as it was. Keeping every marker
instead would blow up the output on series that are mostly null, such
as the climate heating dataset, which went from 823 to 14525 points
before this was bounded.

Skipping markers before the numeric work also makes gapped series
faster: 16% on a series with a few gaps, 27% on one that is mostly
gaps. Both now have benchmark coverage, which the gap path lacked.

Mean mode no longer averages markers in as zero.

src/components/chart/down-sample.ts

@@ -10,12 +10,16 @@ interface MeanFrame {
 }
 
 interface MinMaxFrame {
+  // A frame can hold a gap marker before any value lands in it, so the min/max
+  // slots below only mean something once this is true.
+  hasValue: boolean;
   minPoint: Point;
   minX: number;
   minY: number;
   maxPoint: Point;
   maxX: number;
   maxY: number;
+  gapPoint: Point | undefined;
 }
 
 const SECOND = 1000;
@@ -49,6 +53,25 @@ function snapFrameSize(step: number): number {
   return snapped;
 }
 
+// y is NaN for a frame seeded by a gap marker, which has no value yet.
+function newFrame(
+  point: Point,
+  x: number,
+  y: number,
+  gapPoint: Point | undefined
+): MinMaxFrame {
+  return {
+    hasValue: gapPoint === undefined,
+    minPoint: point,
+    minX: x,
+    minY: y,
+    maxPoint: point,
+    maxX: x,
+    maxY: y,
+    gapPoint,
+  };
+}
+
 export function downSampleLineData<
   T extends [number, number] | NonNullable<LineSeriesOption["data"]>[number],
 >(
@@ -82,7 +105,10 @@ export function downSampleLineData<
       const pointData = getPointData(point);
       if (!Array.isArray(pointData)) continue;
       const x = Number(pointData[0]);
-      const y = Number(pointData[1]);
+      const rawY = pointData[1] as number | null;
+      // Number(null) is 0, which would drag the mean towards zero
+      if (rawY === null) continue;
+      const y = Number(rawY);
       if (isNaN(x) || isNaN(y)) continue;
 
       const frameIndex = Math.floor(x / step);
@@ -120,21 +146,34 @@ export function downSampleLineData<
     const pointData = getPointData(point);
     if (!Array.isArray(pointData)) continue;
     const x = Number(pointData[0]);
-    const y = Number(pointData[1]);
-    if (isNaN(x) || isNaN(y)) continue;
+    if (isNaN(x)) continue;
+    const rawY = pointData[1] as number | null;
+    if (rawY === null) {
+      // The chart data modules push a null value to break the line where an
+      // entity was unavailable. Number(null) is 0, so such a marker must stay
+      // out of the comparisons below, where it would win the minimum slot
+      // whenever the readings are positive and discard the frame's real
+      // minimum. One marker per frame is enough to break the line, and keeping
+      // them all would blow up the output on series that are mostly null. The
+      // last one wins: where the break lands only depends on which points it
+      // sits between, not on its own x.
+      const gapIndex = Math.floor(x / step);
+      const gapFrame = frames.get(gapIndex);
+      if (gapFrame) {
+        gapFrame.gapPoint = point;
+      } else {
+        frames.set(gapIndex, newFrame(point, x, NaN, point));
+      }
+      continue;
+    }
+    const y = Number(rawY);
+    if (isNaN(y)) continue;
 
     const frameIndex = Math.floor(x / step);
     const frame = frames.get(frameIndex);
     if (!frame) {
-      frames.set(frameIndex, {
-        minPoint: point,
-        minX: x,
-        minY: y,
-        maxPoint: point,
-        maxX: x,
-        maxY: y,
-      });
-    } else {
+      frames.set(frameIndex, newFrame(point, x, y, undefined));
+    } else if (frame.hasValue) {
       // Match the original strict-less / strict-greater comparisons so the
       // first occurrence wins on ties.
       if (y < frame.minY) {
@@ -147,18 +186,40 @@ export function downSampleLineData<
         frame.maxX = x;
         frame.maxY = y;
       }
+    } else {
+      // the frame held nothing but a marker so far
+      frame.hasValue = true;
+      frame.minPoint = point;
+      frame.minX = x;
+      frame.minY = y;
+      frame.maxPoint = point;
+      frame.maxX = x;
+      frame.maxY = y;
     }
   }
 
   const result: T[] = [];
   for (const frame of frames.values()) {
-    // The order of the data must be preserved so max may be before min
-    if (frame.minX > frame.maxX) {
-      result.push(frame.maxPoint as T);
+    if (frame.hasValue) {
+      // The order of the data must be preserved so max may be before min
+      if (frame.minX > frame.maxX) {
+        result.push(frame.maxPoint as T);
+      }
+      result.push(frame.minPoint as T);
+      if (frame.minX < frame.maxX) {
+        result.push(frame.maxPoint as T);
+      }
     }
-    result.push(frame.minPoint as T);
-    if (frame.minX < frame.maxX) {
-      result.push(frame.maxPoint as T);
+    if (frame.gapPoint !== undefined) {
+      // A marker followed by a value in its own frame is a gap that closed
+      // within one frame, which is about one device pixel: too narrow to show.
+      // The kept points are exactly min and max, so comparing against the
+      // later of the two catches that without any work on the ingest path. A
+      // marker-only frame compares against its own x and always passes.
+      const lastValueX = frame.minX > frame.maxX ? frame.minX : frame.maxX;
+      if (Number(getPointData(frame.gapPoint)[0]) >= lastValueX) {
+        result.push(frame.gapPoint as T);
+      }
     }
   }
 

test/benchmarks/down-sample.bench.ts

@@ -17,10 +17,24 @@ const generatePoints = (seed: number, count: number): [number, number][] => {
   return points;
 };
 
+// The chart data modules break the line with a null value. A handful of them
+// stands for an entity that went unavailable; a series that is mostly null
+// stands for the climate heating dataset, which emits one per inactive state.
+const withGaps = (
+  points: [number, number][],
+  isGap: (index: number) => boolean
+): [number, number | null][] =>
+  points.map(([x, y], index) => (isGap(index) ? [x, null] : [x, y]));
+
 const small = generatePoints(1, SCALES.small);
 const medium = generatePoints(2, SCALES.medium);
 const large = generatePoints(3, SCALES.large);
 const largeObjects = large.map((value) => ({ value }));
+const largeFewGaps = withGaps(large, (index) => index % 20_000 === 0);
+const largeMostlyGaps = withGaps(
+  large,
+  (index) => Math.floor(index / 50) % 3 !== 0
+);
 
 describe("downSampleLineData", () => {
   bench("min/max small (1k points)", () => {
@@ -54,4 +68,20 @@ describe("downSampleLineData", () => {
     },
     { time: 1000, warmupIterations: 2 }
   );
+
+  bench(
+    "min/max large with a few gaps (100k points)",
+    () => {
+      downSampleLineData(largeFewGaps, MAX_DETAILS);
+    },
+    { time: 1000, warmupIterations: 2 }
+  );
+
+  bench(
+    "min/max large mostly gaps (100k points)",
+    () => {
+      downSampleLineData(largeMostlyGaps, MAX_DETAILS);
+    },
+    { time: 1000, warmupIterations: 2 }
+  );
 });

test/components/chart/down-sample.test.ts

@@ -19,9 +19,52 @@ const generatePoints = (
   return points;
 };
 
-const toObjectPoints = (points: [number, number][]) =>
+// Gap markers: the chart data modules push a null value to break the line
+// where an entity was unavailable.
+type GappedPoint = [number, number | null | undefined];
+
+const toObjectPoints = (points: GappedPoint[]) =>
   points.map((value) => ({ value }));
 
+const expectXOrdered = (result: { [0]: number }[]) => {
+  for (let i = 1; i < result.length; i++) {
+    expect(result[i][0]).toBeGreaterThanOrEqual(result[i - 1][0]);
+  }
+};
+
+// A series whose readings are all positive, with an unavailable stretch that
+// starts inside the first frame. Mirrors the point sequence
+// state-history-chart-line-data.ts emits for a gap.
+const gappedPoints: GappedPoint[] = [
+  [FIXED_EPOCH_MS, 50],
+  [FIXED_EPOCH_MS + 1_000, 90], // frame maximum
+  [FIXED_EPOCH_MS + 2_000, 60],
+  [FIXED_EPOCH_MS + 3_000, 10], // frame minimum
+  [FIXED_EPOCH_MS + 4_000, 20], // last reading before the gap
+  [FIXED_EPOCH_MS + 4_001, null], // gap marker
+  [FIXED_EPOCH_MS + 30_000, 55],
+  [FIXED_EPOCH_MS + 31_000, 45],
+];
+
+// A generated series with three unavailable stretches of different lengths.
+const generateGappedPoints = (seed: number, count: number) => {
+  const points: GappedPoint[] = generatePoints(seed, count);
+  for (const [start, length] of [
+    [Math.floor(count * 0.13), 3],
+    [Math.floor(count * 0.4), 25],
+    [Math.floor(count * 0.83), 1],
+  ]) {
+    const gapStart = points[start][0];
+    points.splice(
+      start + 1,
+      length,
+      [gapStart + 1, points[start][1]],
+      [gapStart + 1, null]
+    );
+  }
+  return points;
+};
+
 describe("downSampleLineData", () => {
   it("returns empty array for undefined data", () => {
     expect(downSampleLineData(undefined, 100)).toEqual([]);
@@ -66,11 +109,7 @@ describe("downSampleLineData", () => {
   });
 
   it("min/max mode preserves x-order for sorted input", () => {
-    const points = generatePoints(4, 1000);
-    const result = downSampleLineData(points, 50);
-    for (let i = 1; i < result.length; i++) {
-      expect(result[i][0]).toBeGreaterThanOrEqual(result[i - 1][0]);
-    }
+    expectXOrdered(downSampleLineData(generatePoints(4, 1000), 50));
   });
 
   it("min/max mode matches characterization snapshot", () => {
@@ -193,6 +232,165 @@ describe("downSampleLineData", () => {
     ).toMatchSnapshot();
   });
 
+  it("keeps the frame minimum when a gap marker shares the frame", () => {
+    // Without special handling the marker becomes y=0, wins the minimum slot
+    // and the real minimum (10) is dropped.
+    expect(downSampleLineData(gappedPoints, 3)).toEqual([
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 3_000, 10],
+      [FIXED_EPOCH_MS + 4_001, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ]);
+  });
+
+  it("drops a marker whose gap closes within the same frame", () => {
+    // A frame spans about one device pixel, so a gap that opens and closes
+    // inside one is too narrow to show. The values around it stay.
+    const points: GappedPoint[] = [
+      [FIXED_EPOCH_MS, 50],
+      [FIXED_EPOCH_MS + 1_000, 10], // frame minimum, before the marker
+      [FIXED_EPOCH_MS + 1_001, null],
+      [FIXED_EPOCH_MS + 2_000, 90], // frame maximum, after the marker
+      [FIXED_EPOCH_MS + 3_000, 60],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ];
+    expect(downSampleLineData(points, 3)).toEqual([
+      [FIXED_EPOCH_MS + 1_000, 10],
+      [FIXED_EPOCH_MS + 2_000, 90],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ]);
+  });
+
+  it("keeps a marker sharing its x with a value after that value", () => {
+    // statistics-chart-data.ts ends the line and breaks it at the same x
+    const points: GappedPoint[] = [
+      [FIXED_EPOCH_MS, 50],
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 2_000, 10],
+      [FIXED_EPOCH_MS + 2_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ];
+    expect(downSampleLineData(points, 3)).toEqual([
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 2_000, 10],
+      [FIXED_EPOCH_MS + 2_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ]);
+  });
+
+  it("keeps a gap marker whose frame holds no values", () => {
+    const points: GappedPoint[] = [
+      [FIXED_EPOCH_MS, 50],
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 2_000, 60],
+      [FIXED_EPOCH_MS + 15_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ];
+    expect(downSampleLineData(points, 3)).toEqual([
+      [FIXED_EPOCH_MS, 50],
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 15_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ]);
+  });
+
+  it("keeps a single gap marker per frame", () => {
+    // A run of nulls inside one frame renders the same as a single null: the
+    // break only depends on which points the marker sits between.
+    const points: GappedPoint[] = [
+      [FIXED_EPOCH_MS, 50],
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 2_000, 10],
+      [FIXED_EPOCH_MS + 3_000, null],
+      [FIXED_EPOCH_MS + 4_000, null],
+      [FIXED_EPOCH_MS + 5_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ];
+    expect(downSampleLineData(points, 3)).toEqual([
+      [FIXED_EPOCH_MS + 1_000, 90],
+      [FIXED_EPOCH_MS + 2_000, 10],
+      [FIXED_EPOCH_MS + 5_000, null],
+      [FIXED_EPOCH_MS + 30_000, 55],
+      [FIXED_EPOCH_MS + 31_000, 45],
+    ]);
+  });
+
+  it("bounds the output on a series where most points are null", () => {
+    // The climate heating/cooling datasets push a null for every state where
+    // the mode is inactive, so markers must not escape the frame budget.
+    const values = generatePoints(20, SCALES.medium);
+    const random = createSeededRandom(21);
+    const gapped: GappedPoint[] = values.map(([x, y]) =>
+      random() < 0.65 ? [x, null] : [x, y]
+    );
+    const gapless = downSampleLineData(values, 500);
+    const result = downSampleLineData(gapped, 500);
+    // Both series share an x grid, so they share frames, and the gapless one
+    // emits at least one point per frame. A gapped frame emits at most three:
+    // min, max and a single marker.
+    expect(result.length).toBeLessThanOrEqual(3 * gapless.length);
+    expect(
+      result.filter((point) => point[1] === null).length
+    ).toBeLessThanOrEqual(gapless.length);
+  });
+
+  it("handles gap markers on object-shaped points", () => {
+    const points = toObjectPoints(gappedPoints);
+    expect(downSampleLineData(points, 3)).toEqual([
+      points[1],
+      points[3],
+      points[5],
+      points[6],
+      points[7],
+    ]);
+  });
+
+  it("handles gap markers on Date x values", () => {
+    // statistics charts use Date objects for x
+    const points = gappedPoints.map(
+      ([x, y]) => [new Date(x), y] as [Date, number | null | undefined]
+    );
+    expect(downSampleLineData(points, 3)).toEqual([
+      points[1],
+      points[3],
+      points[5],
+      points[6],
+      points[7],
+    ]);
+  });
+
+  it("mean mode leaves gap markers out of the average", () => {
+    const points: GappedPoint[] = [
+      [FIXED_EPOCH_MS, 10],
+      [FIXED_EPOCH_MS + 1_000, 20],
+      [FIXED_EPOCH_MS + 1_001, null],
+      [FIXED_EPOCH_MS + 2_000, 30],
+      [FIXED_EPOCH_MS + 30_000, 100],
+      [FIXED_EPOCH_MS + 31_000, 100],
+    ];
+    expect(downSampleLineData(points, 3, undefined, undefined, true)).toEqual([
+      // (10 + 20 + 30) / 3, not (10 + 20 + 0 + 30) / 4
+      [FIXED_EPOCH_MS + 1_000, 20],
+      [FIXED_EPOCH_MS + 30_500, 100],
+    ]);
+  });
+
+  it("min/max mode preserves x-order for gapped input", () => {
+    const result = downSampleLineData(generateGappedPoints(22, 1000), 50);
+    // Of the three gaps only the 25 point one outlasts its frame; the one and
+    // three point gaps close within theirs and are dropped.
+    expect(result.filter((point) => point[1] === null)).toHaveLength(1);
+    expectXOrdered(result);
+  });
+
   it("large scale mean-mode digest is stable", () => {
     expect(
       digestResult(