From d507005a0a203ad3ee8a1ec032f276fbc8273a3c Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Mon, 23 Dec 2024 18:20:13 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A8=20refactor=20legacyToOwidTableAndD?= =?UTF-8?q?imensions=20to=20be=20chart=20type=20specific?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/core/Grapher.tsx | 5 +- .../src/core/LegacyToOwidTable.test.ts | 74 ++++++++++--------- .../grapher/src/core/LegacyToOwidTable.ts | 68 ++++++++--------- 3 files changed, 75 insertions(+), 72 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 8cbd4e22d1f..f58bbbb30c3 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -1145,10 +1145,11 @@ export class Grapher // TODO grapher model: switch this to downloading multiple data and metadata files const startMark = performance.now() - const { dimensions, table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( json, - legacyConfig + legacyConfig.dimensions ?? [] ) + const dimensions = legacyConfig.dimensions ?? [] this.createPerformanceMeasurement( "legacyToOwidTableAndDimensions", startMark diff --git a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts index c7cac1c958b..2766a107bbc 100755 --- a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts +++ b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts @@ -8,7 +8,10 @@ import { LegacyGrapherInterface, } from "@ourworldindata/types" import { ColumnTypeMap, ErrorValueTypes } from "@ourworldindata/core-table" -import { legacyToOwidTableAndDimensions } from "./LegacyToOwidTable" +import { + addSelectedEntityColorsToTable, + legacyToOwidTableAndDimensions, +} from "./LegacyToOwidTable" import { MultipleOwidVariableDataDimensionsMap, OwidVariableDataMetadataDimensions, @@ -43,9 +46,9 @@ describe(legacyToOwidTableAndDimensions, () => { } it("contains the standard entity columns", () => { - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) expect(table.columnSlugs).toEqual( expect.arrayContaining( @@ -62,27 +65,22 @@ describe(legacyToOwidTableAndDimensions, () => { describe("conversionFactor", () => { it("applies the more specific chart-level conversionFactor", () => { - const { table } = legacyToOwidTableAndDimensions( - legacyVariableConfig, + const table = legacyToOwidTableAndDimensions(legacyVariableConfig, [ { - dimensions: [ - { - variableId: 2, - display: { conversionFactor: 10 }, - property: DimensionProperty.y, - }, - ], - } - ) + variableId: 2, + display: { conversionFactor: 10 }, + property: DimensionProperty.y, + }, + ]) // Apply the chart-level conversionFactor (10) expect(table.rows[0]["2"]).toEqual(80) }) it("applies the more variable-level conversionFactor if a chart-level one is not present", () => { - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) // Apply the variable-level conversionFactor (100) @@ -172,9 +170,9 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) it("leaves ErrorValues when there were no values to join to", () => { @@ -315,9 +313,9 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) it("shifts values in days array when zeroDay is is not EPOCH_DATE", () => { @@ -438,9 +436,9 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) it("duplicates 'day' column into 'time'", () => { @@ -469,9 +467,9 @@ describe(legacyToOwidTableAndDimensions, () => { chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot], } - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - scatterLegacyGrapherConfig + scatterLegacyGrapherConfig.dimensions ?? [] ) expect(table.rows.length).toEqual(3) @@ -627,9 +625,9 @@ describe("variables with mixed days & years with missing overlap and multiple po ], } - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) it("duplicates 'day' column into 'time'", () => { @@ -646,9 +644,9 @@ describe("variables with mixed days & years with missing overlap and multiple po describe("join behaviour without target times is sane", () => { it("creates a sane table join", () => { - const { table } = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensions( legacyVariableConfig, - legacyGrapherConfig + legacyGrapherConfig.dimensions ?? [] ) // A sane join between years and days would create 5 days for the given input @@ -764,10 +762,18 @@ const getLegacyGrapherConfig = (): Partial => { } describe("creating a table from legacy", () => { - const table = legacyToOwidTableAndDimensions(getOwidVarSet(), { + const config = { ...getLegacyGrapherConfig(), selectedEntityColors: { "Cape Verde": "blue" }, - }).table + } + const table = addSelectedEntityColorsToTable( + legacyToOwidTableAndDimensions( + getOwidVarSet(), + config.dimensions ?? [] + ), + config.selectedEntityColors + ) + const name = "Prevalence of wasting, weight for height (% of children under 5)" @@ -802,8 +808,8 @@ describe("creating a table from legacy", () => { expect( legacyToOwidTableAndDimensions( varSet, - getLegacyGrapherConfig() - ).table.get("3512")!.values + getLegacyGrapherConfig().dimensions ?? [] + ).get("3512")!.values ).toEqual([550, 420, 1260]) }) @@ -825,8 +831,8 @@ Papua New Guinea,PNG,1983,5.5` varSet.get(3512)!.metadata.nonRedistributable = true const columnDef = legacyToOwidTableAndDimensions( varSet, - getLegacyGrapherConfig() - ).table.get("3512").def as OwidColumnDef + getLegacyGrapherConfig().dimensions ?? [] + ).get("3512").def as OwidColumnDef expect(columnDef.nonRedistributable).toEqual(true) }) }) diff --git a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts index 56039ca16b6..47f6eafdda7 100644 --- a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts +++ b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts @@ -1,15 +1,14 @@ // todo: Remove this file when we've migrated OWID data and OWID charts to next version import { - GRAPHER_CHART_TYPES, ColumnTypeNames, CoreColumnDef, StandardOwidColumnDefs, OwidTableSlugs, OwidColumnDef, - LegacyGrapherInterface, OwidVariableDimensions, OwidVariableDataMetadataDimensions, + EntityId, } from "@ourworldindata/types" import { OwidTable, @@ -42,8 +41,8 @@ import { isContinentsVariableId } from "./GrapherConstants" export const legacyToOwidTableAndDimensions = ( json: MultipleOwidVariableDataDimensionsMap, - grapherConfig: Partial -): { dimensions: OwidChartDimensionInterface[]; table: OwidTable } => { + dimensions: OwidChartDimensionInterface[] +): OwidTable => { // Entity meta map const entityMeta = [...json.values()].flatMap( @@ -53,8 +52,6 @@ export const legacyToOwidTableAndDimensions = ( entityMeta.map((entity) => [entity.id.toString(), entity]) ) - const dimensions = grapherConfig.dimensions || [] - // Base column defs, shared by all variable tables const baseColumnDefs: Map = new Map() @@ -62,17 +59,6 @@ export const legacyToOwidTableAndDimensions = ( baseColumnDefs.set(def.slug, def) }) - const entityColorColumnSlug = grapherConfig.selectedEntityColors - ? OwidTableSlugs.entityColor - : undefined - if (entityColorColumnSlug) { - baseColumnDefs.set(entityColorColumnSlug, { - slug: entityColorColumnSlug, - type: ColumnTypeNames.Color, - name: entityColorColumnSlug, - }) - } - // We need to create a column for each unique [variable, targetTime] pair. So there can be // multiple columns for a single variable. const newDimensions = dimensions.map((dimension) => ({ @@ -174,18 +160,6 @@ export const legacyToOwidTableAndDimensions = ( ) columnDefs.set(annotationColumnDef.slug, annotationColumnDef) } - - if (entityColorColumnSlug) { - columnStore[entityColorColumnSlug] = entityIds.map((entityId) => { - // see comment above about entityMetaById[id] - const entityName = entityMetaById[entityId]?.name - const selectedEntityColors = grapherConfig.selectedEntityColors - return entityName && selectedEntityColors - ? selectedEntityColors[entityName] - : undefined - }) - } - // Build the tables let variableTable = new OwidTable( @@ -198,12 +172,7 @@ export const legacyToOwidTableAndDimensions = ( // We do this by dropping the column. We interpolate before which adds an originalTime // column which can be used to recover the time. const targetTime = dimension?.targetYear - const chartType = grapherConfig.chartTypes?.[0] - if ( - (chartType === GRAPHER_CHART_TYPES.ScatterPlot || - chartType === GRAPHER_CHART_TYPES.Marimekko) && - isNumber(targetTime) - ) { + if (isNumber(targetTime)) { variableTable = variableTable // interpolateColumnWithTolerance() won't handle injecting times beyond the current // allTimes. So if targetYear is 2018, and we have data up to 2017, the @@ -359,7 +328,34 @@ export const legacyToOwidTableAndDimensions = ( } } - return { dimensions: newDimensions, table: joinedVariablesTable } + return joinedVariablesTable +} + +export const addSelectedEntityColorsToTable = ( + table: OwidTable, + selectedEntityColors: { [entityName: string]: string } +): OwidTable => { + const entityColorColumnSlug = OwidTableSlugs.entityColor + + const valueFn = (entityId: EntityId | undefined) => { + // see comment above about entityMetaById[id] + if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString + const entityName = table.entityIdToNameMap.get(entityId) + return entityName && selectedEntityColors + ? selectedEntityColors[entityName] + : ErrorValueTypes.UndefinedButShouldBeString + } + + const values = table.rows.map((row) => valueFn(row.entityId)) + + return table.appendColumns([ + { + slug: entityColorColumnSlug, + name: entityColorColumnSlug, + type: ColumnTypeNames.Color, + values: values, + }, + ]) } const fullJoinTables = (