🐛 (scatter/marimekko) apply color/size tolerance early

owid · Dec 18, 2024 · 0a3f0f4 · 0a3f0f4
1 parent cf54ea1
commit 0a3f0f4
Show file tree

Hide file tree

Showing 4 changed files with 124 additions and 65 deletions.
diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx
@@ -829,9 +829,47 @@ export class Grapher
         return table
     }
 
+    /**
+     * Input table with color and size tolerance applied.
+     *
+     * This happens _before_ applying the author's timeline filter to avoid
+     * accidentally dropping all color values before applying tolerance.
+     * This is especially important for scatter plots and Marimekko charts,
+     * where color and size columns are often transformed with infinite tolerance.
+     *
+     * Line and discrete bar charts also support a color dimension, but their
+     * tolerance transformations run in their respective transformTable functions
+     * since it's more efficient to run them on a table that has been filtered
+     * by selected entities.
+     */
+    @computed get tableAfterColorAndSizeToleranceApplication(): OwidTable {
+        let table = this.inputTable
+
+        if (this.isScatter && this.sizeColumnSlug) {
+            const tolerance =
+                table.get(this.sizeColumnSlug)?.display?.tolerance ?? Infinity
+            table = table.interpolateColumnWithTolerance(
+                this.sizeColumnSlug,
+                tolerance
+            )
+        }
+
+        if ((this.isScatter || this.isMarimekko) && this.colorColumnSlug) {
+            const tolerance =
+                table.get(this.colorColumnSlug)?.display?.tolerance ?? Infinity
+            table = table.interpolateColumnWithTolerance(
+                this.colorColumnSlug,
+                tolerance
+            )
+        }
+
+        return table
+    }
+
     // If an author sets a timeline filter run it early in the pipeline so to the charts it's as if the filtered times do not exist
     @computed get tableAfterAuthorTimelineFilter(): OwidTable {
-        const table = this.inputTable
+        const table = this.tableAfterColorAndSizeToleranceApplication
+
         if (
             this.timelineMinTime === undefined &&
             this.timelineMaxTime === undefined

diff --git a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.test.ts b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.test.ts
@@ -22,10 +22,12 @@ import {
     ColumnTypeNames,
     OwidTableSlugs,
     Color,
+    GRAPHER_CHART_TYPES,
 } from "@ourworldindata/types"
 import { ContinentColors } from "../color/CustomSchemes"
 import { sortBy, uniq, uniqBy } from "@ourworldindata/utils"
 import { ScatterPointsWithLabels } from "./ScatterPointsWithLabels"
+import { Grapher } from "../core/Grapher"
 
 it("can create a new chart", () => {
     const manager: ScatterPlotManager = {
@@ -139,14 +141,16 @@ describe("interpolation defaults", () => {
             },
         ]
     )
-    const manager: ScatterPlotManager = {
-        xColumnSlug: "x",
-        yColumnSlug: "y",
-        colorColumnSlug: "color",
-        sizeColumnSlug: "size",
+
+    const grapher = new Grapher({
         table,
-    }
-    const chart = new ScatterPlotChart({ manager })
+        chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
+        xSlug: "x",
+        ySlugs: "y",
+        colorSlug: "color",
+        sizeSlug: "size",
+    })
+    const chart = grapher.chartInstance as ScatterPlotChart
 
     it("color defaults to infinity tolerance if none specified", () => {
         expect(
@@ -195,15 +199,15 @@ describe("basic scatterplot", () => {
         ]
     )
 
-    const manager: ScatterPlotManager = {
-        xColumnSlug: "x",
-        yColumnSlug: "y",
-        colorColumnSlug: "color",
-        sizeColumnSlug: "size",
+    const grapher = new Grapher({
+        chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
+        xSlug: "x",
+        ySlugs: "y",
+        colorSlug: "color",
+        sizeSlug: "size",
         table,
-    }
-
-    const chart = new ScatterPlotChart({ manager })
+    })
+    const chart = grapher.chartInstance as ScatterPlotChart
 
     it("removes error values from X and Y", () => {
         expect(chart.transformedTable.numRows).toEqual(2)
@@ -425,16 +429,16 @@ describe("entity exclusion", () => {
         ]
     )
 
-    const manager: ScatterPlotManager = {
-        xColumnSlug: "x",
-        yColumnSlug: "y",
-        colorColumnSlug: "color",
-        sizeColumnSlug: "size",
+    const grapher = new Grapher({
+        chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
+        xSlug: "x",
+        ySlugs: "y",
+        colorSlug: "color",
+        sizeSlug: "size",
         matchingEntitiesOnly: true,
         table,
-    }
-
-    const chart = new ScatterPlotChart({ manager })
+    })
+    const chart = grapher.chartInstance as ScatterPlotChart
 
     it("excludes entities without color when matchingEntitiesOnly is enabled", () => {
         expect(chart.allPoints.length).toEqual(1)
@@ -815,15 +819,15 @@ describe("x/y tolerance", () => {
         ]
     )
 
-    const manager: ScatterPlotManager = {
-        xColumnSlug: "x",
-        yColumnSlug: "y",
-        colorColumnSlug: "color",
-        sizeColumnSlug: "size",
+    const grapher = new Grapher({
+        chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
+        xSlug: "x",
+        ySlugs: "y",
+        colorSlug: "color",
+        sizeSlug: "size",
         table,
-    }
-
-    const chart = new ScatterPlotChart({ manager })
+    })
+    const chart = grapher.chartInstance as ScatterPlotChart
 
     const transformedTable = chart.transformedTable
 
@@ -1030,3 +1034,48 @@ describe("correct bubble sizes", () => {
         expect(sortedRenderSeries[1].fontSize).toEqual(10.5)
     })
 })
+
+it("applies color tolerance before applying the author timeline filter", () => {
+    const table = new OwidTable(
+        [
+            [
+                "entityId",
+                "entityName",
+                "entityCode",
+                "year",
+                "x",
+                "y",
+                "color",
+                "size",
+            ],
+            [1, "UK", "", -1000, 1, 1, null, null],
+            [1, "UK", "", 1000, 1, 1, null, 100],
+            [1, "UK", "", 2020, 1, 1, null, null],
+            [1, "UK", "", 2023, null, null, "Europe", null],
+        ],
+        [
+            { slug: "x", type: ColumnTypeNames.Numeric },
+            { slug: "y", type: ColumnTypeNames.Numeric },
+            { slug: "color", type: ColumnTypeNames.String },
+            {
+                slug: "size",
+                type: ColumnTypeNames.Numeric,
+            },
+        ]
+    )
+
+    const grapher = new Grapher({
+        table,
+        chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
+        xSlug: "x",
+        ySlugs: "y",
+        colorSlug: "color",
+        sizeSlug: "size",
+        timelineMaxTime: 2020,
+    })
+    const chart = grapher.chartInstance as ScatterPlotChart
+
+    expect(
+        uniq(chart.transformedTable.get("color").valuesIncludingErrorValues)
+    ).toEqual(["Europe"])
+})
diff --git a/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx b/packages/@ourworldindata/grapher/src/scatterCharts/ScatterPlotChart.tsx
@@ -171,28 +171,8 @@ export class ScatterPlotChart
         if (this.yScaleType === ScaleType.log && this.yColumnSlug)
             table = table.replaceNonPositiveCellsForLogScale([this.yColumnSlug])
 
-        if (this.sizeColumnSlug) {
-            const tolerance =
-                table.get(this.sizeColumnSlug)?.display?.tolerance ?? Infinity
-            table = table.interpolateColumnWithTolerance(
-                this.sizeColumnSlug,
-                tolerance
-            )
-        }
-
-        if (this.colorColumnSlug) {
-            const tolerance =
-                table.get(this.colorColumnSlug)?.display?.tolerance ?? Infinity
-            table = table.interpolateColumnWithTolerance(
-                this.colorColumnSlug,
-                tolerance
-            )
-            if (this.manager.matchingEntitiesOnly) {
-                table = table.dropRowsWithErrorValuesForColumn(
-                    this.colorColumnSlug
-                )
-            }
-        }
+        if (this.colorColumnSlug && this.manager.matchingEntitiesOnly)
+            table = table.dropRowsWithErrorValuesForColumn(this.colorColumnSlug)
 
         // We want to "chop off" any rows outside the time domain for X and Y to avoid creating
         // leading and trailing timeline times that don't really exist in the dataset.

diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx
@@ -318,17 +318,9 @@ export class MarimekkoChart
         if (xColumnSlug)
             table = table.interpolateColumnWithTolerance(xColumnSlug)
 
-        if (colorColumnSlug) {
-            const tolerance =
-                table.get(colorColumnSlug)?.display?.tolerance ?? Infinity
-            table = table.interpolateColumnWithTolerance(
-                colorColumnSlug,
-                tolerance
-            )
-            if (manager.matchingEntitiesOnly) {
-                table = table.dropRowsWithErrorValuesForColumn(colorColumnSlug)
-            }
-        }
+        if (colorColumnSlug && manager.matchingEntitiesOnly)
+            table = table.dropRowsWithErrorValuesForColumn(colorColumnSlug)
+
         if (!manager.showNoDataArea)
             table = table.dropRowsWithErrorValuesForAllColumns(yColumnSlugs)