From 3cdecdcfcd0c9b07e9f876ca88e7b9a28e40e96f Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Mon, 23 Dec 2024 20:11:08 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=94=A8=20make=20sure=20to=20add=20sel?= =?UTF-8?q?ected=20entity=20colors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/core/Grapher.tsx | 25 ++++-- .../src/core/LegacyToOwidTable.test.ts | 74 ++++++++-------- .../grapher/src/core/LegacyToOwidTable.ts | 86 ++++++++++--------- 3 files changed, 103 insertions(+), 82 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 6c1d75e81f7..0fa28237c7c 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -193,7 +193,11 @@ import { DefaultChartClass, } from "../chart/ChartTypeMap" import { Entity, SelectionArray } from "../selection/SelectionArray" -import { legacyToOwidTableAndDimensions } from "./LegacyToOwidTable" +import { + addSelectedEntityColorsToTable, + computeActualDimensions, + legacyToOwidTableAndDimensions, +} from "./LegacyToOwidTable" import { ScatterPlotManager } from "../scatterCharts/ScatterPlotChartConstants" import { autoDetectSeriesStrategy, @@ -1179,18 +1183,27 @@ 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 tableWithColors = legacyConfig.selectedEntityColors + ? addSelectedEntityColorsToTable( + table, + legacyConfig.selectedEntityColors + ) + : table + const dimensions = legacyConfig.dimensions + ? computeActualDimensions(legacyConfig.dimensions) + : [] this.createPerformanceMeasurement( "legacyToOwidTableAndDimensions", startMark ) if (inputTableTransformer) - this.inputTable = inputTableTransformer(table) - else this.inputTable = table + this.inputTable = inputTableTransformer(tableWithColors) + else this.inputTable = tableWithColors // We need to reset the dimensions because some of them may have changed slugs in the legacy // transformation (can happen when columns use targetTime) 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..79b4fa25e8f 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,25 +59,9 @@ 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) => ({ - ...dimension, - slug: dimension.targetYear - ? `${dimension.variableId}-${dimension.targetYear}` - : `${dimension.variableId}`, - })) + const newDimensions = computeActualDimensions(dimensions) const dimensionColumns = uniqBy(newDimensions, (dim) => dim.slug) const variableTablesToJoinByYear: OwidTable[] = [] @@ -174,18 +155,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 +167,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 +323,34 @@ export const legacyToOwidTableAndDimensions = ( } } - return { dimensions: newDimensions, table: joinedVariablesTable } + return joinedVariablesTable +} + +export const addSelectedEntityColorsToTable = ( + table: OwidTable, + selectedEntityColors: { [entityName: string]: string | undefined } +): OwidTable => { + const entityColorColumnSlug = OwidTableSlugs.entityColor + + const valueFn = (entityId: EntityId | undefined) => { + if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString + const entityName = table.entityIdToNameMap.get(entityId) + return entityName && selectedEntityColors + ? (selectedEntityColors[entityName] ?? + ErrorValueTypes.UndefinedButShouldBeString) + : 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 = ( @@ -747,6 +738,17 @@ const annotationsToMap = (annotations: string): Map => { return entityAnnotationsMap } +export function computeActualDimensions( + dimensions: OwidChartDimensionInterface[] +): OwidChartDimensionInterface[] { + return dimensions.map((dimension) => ({ + ...dimension, + slug: dimension.targetYear + ? `${dimension.variableId}-${dimension.targetYear}` + : `${dimension.variableId}`, + })) +} + /** * Loads a single variable into an OwidTable. */ From c1d8bfdd822a488d8185dd8ec7ce5cdaeab3d93a Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Fri, 10 Jan 2025 16:27:31 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=94=A8=20simplify=20working=20with=20?= =?UTF-8?q?the=20new=20legacyToOwidTable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/chart/ChartDimension.ts | 22 +++- .../grapher/src/core/Grapher.tsx | 36 +++--- .../src/core/LegacyToOwidTable.test.ts | 111 ++++++++++++------ .../grapher/src/core/LegacyToOwidTable.ts | 77 ++++++------ .../src/OwidVariableDisplayConfigInterface.ts | 5 + packages/@ourworldindata/types/src/index.ts | 1 + 6 files changed, 150 insertions(+), 102 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts b/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts index 370cc103273..c5af906e291 100644 --- a/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts +++ b/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts @@ -13,6 +13,7 @@ import { OwidVariableDisplayConfig, OwidChartDimensionInterface, Time, + OwidChartDimensionInterfaceWithMandatorySlug, } from "@ourworldindata/utils" import { OwidTable, CoreColumn } from "@ourworldindata/core-table" @@ -35,9 +36,17 @@ export interface LegacyDimensionsManager { table: OwidTable } +export function getDimensionColumnSlug( + variableId: OwidVariableId, + targetYear: Time | undefined +): ColumnSlug { + if (targetYear) return `${variableId}-${targetYear}` + return variableId.toString() +} + export class ChartDimension extends ChartDimensionDefaults - implements Persistable + implements Persistable, OwidChartDimensionInterfaceWithMandatorySlug { private manager: LegacyDimensionsManager @@ -78,7 +87,16 @@ export class ChartDimension } // Do not persist yet, until we migrate off VariableIds - @observable slug?: ColumnSlug + @observable _slug?: ColumnSlug | undefined + + @computed get slug(): ColumnSlug { + if (this._slug) return this._slug + return getDimensionColumnSlug(this.variableId, this.targetYear) + } + + set slug(value: ColumnSlug | undefined) { + this._slug = value + } @computed get column(): CoreColumn { return this.table.get(this.columnSlug) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 0fa28237c7c..994a25f8b23 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -142,6 +142,7 @@ import { loadVariableDataAndMetadata } from "./loadVariable" import Cookies from "js-cookie" import { ChartDimension, + getDimensionColumnSlug, LegacyDimensionsManager, } from "../chart/ChartDimension" import { TooltipManager } from "../tooltip/TooltipProps" @@ -193,11 +194,7 @@ import { DefaultChartClass, } from "../chart/ChartTypeMap" import { Entity, SelectionArray } from "../selection/SelectionArray" -import { - addSelectedEntityColorsToTable, - computeActualDimensions, - legacyToOwidTableAndDimensions, -} from "./LegacyToOwidTable" +import { legacyToOwidTableAndDimensions } from "./LegacyToOwidTable" import { ScatterPlotManager } from "../scatterCharts/ScatterPlotChartConstants" import { autoDetectSeriesStrategy, @@ -1183,19 +1180,20 @@ export class Grapher // TODO grapher model: switch this to downloading multiple data and metadata files const startMark = performance.now() - const table = legacyToOwidTableAndDimensions( + const dimensions = legacyConfig.dimensions?.map((dimension) => ({ + ...dimension, + slug: + dimension.slug ?? + getDimensionColumnSlug( + dimension.variableId, + dimension.targetYear + ), + })) + const tableWithColors = legacyToOwidTableAndDimensions( json, - legacyConfig.dimensions ?? [] - ) - const tableWithColors = legacyConfig.selectedEntityColors - ? addSelectedEntityColorsToTable( - table, - legacyConfig.selectedEntityColors - ) - : table - const dimensions = legacyConfig.dimensions - ? computeActualDimensions(legacyConfig.dimensions) - : [] + dimensions ?? [], + legacyConfig.selectedEntityColors + ) this.createPerformanceMeasurement( "legacyToOwidTableAndDimensions", startMark @@ -1205,10 +1203,6 @@ export class Grapher this.inputTable = inputTableTransformer(tableWithColors) else this.inputTable = tableWithColors - // We need to reset the dimensions because some of them may have changed slugs in the legacy - // transformation (can happen when columns use targetTime) - this.setDimensionsFromConfigs(dimensions) - this.appendNewEntitySelectionOptions() if (this.manager?.selection?.hasSelection) { diff --git a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts index 2766a107bbc..a6bd3265a07 100755 --- a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts +++ b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts @@ -6,17 +6,40 @@ import { OwidTableSlugs, StandardOwidColumnDefs, LegacyGrapherInterface, + OwidChartDimensionInterface, } from "@ourworldindata/types" -import { ColumnTypeMap, ErrorValueTypes } from "@ourworldindata/core-table" import { - addSelectedEntityColorsToTable, - legacyToOwidTableAndDimensions, -} from "./LegacyToOwidTable" + ColumnTypeMap, + ErrorValueTypes, + OwidTable, +} from "@ourworldindata/core-table" +import { legacyToOwidTableAndDimensions } from "./LegacyToOwidTable" import { MultipleOwidVariableDataDimensionsMap, OwidVariableDataMetadataDimensions, DimensionProperty, } from "@ourworldindata/utils" +import { getDimensionColumnSlug } from "../chart/ChartDimension.js" + +export const legacyToOwidTableAndDimensionsWithMandatorySlug = ( + json: MultipleOwidVariableDataDimensionsMap, + dimensions: OwidChartDimensionInterface[], + selectedEntityColors: + | { [entityName: string]: string | undefined } + | undefined +): OwidTable => { + const dimensionsWithSlug = dimensions?.map((dimension) => ({ + ...dimension, + slug: + dimension.slug ?? + getDimensionColumnSlug(dimension.variableId, dimension.targetYear), + })) + return legacyToOwidTableAndDimensions( + json, + dimensionsWithSlug, + selectedEntityColors + ) +} describe(legacyToOwidTableAndDimensions, () => { const legacyVariableEntry: OwidVariableDataMetadataDimensions = { @@ -46,9 +69,10 @@ describe(legacyToOwidTableAndDimensions, () => { } it("contains the standard entity columns", () => { - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) expect(table.columnSlugs).toEqual( expect.arrayContaining( @@ -65,22 +89,27 @@ describe(legacyToOwidTableAndDimensions, () => { describe("conversionFactor", () => { it("applies the more specific chart-level conversionFactor", () => { - const table = legacyToOwidTableAndDimensions(legacyVariableConfig, [ - { - variableId: 2, - display: { conversionFactor: 10 }, - property: DimensionProperty.y, - }, - ]) + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( + legacyVariableConfig, + [ + { + variableId: 2, + display: { conversionFactor: 10 }, + property: DimensionProperty.y, + }, + ], + legacyGrapherConfig.selectedEntityColors + ) // 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 = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) // Apply the variable-level conversionFactor (100) @@ -170,9 +199,10 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) it("leaves ErrorValues when there were no values to join to", () => { @@ -313,9 +343,10 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + {} ) it("shifts values in days array when zeroDay is is not EPOCH_DATE", () => { @@ -436,9 +467,10 @@ describe(legacyToOwidTableAndDimensions, () => { ], } - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) it("duplicates 'day' column into 'time'", () => { @@ -467,9 +499,10 @@ describe(legacyToOwidTableAndDimensions, () => { chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot], } - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - scatterLegacyGrapherConfig.dimensions ?? [] + scatterLegacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) expect(table.rows.length).toEqual(3) @@ -625,9 +658,10 @@ describe("variables with mixed days & years with missing overlap and multiple po ], } - const table = legacyToOwidTableAndDimensions( + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) it("duplicates 'day' column into 'time'", () => { @@ -644,9 +678,10 @@ 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 = legacyToOwidTableAndDimensionsWithMandatorySlug( legacyVariableConfig, - legacyGrapherConfig.dimensions ?? [] + legacyGrapherConfig.dimensions ?? [], + legacyGrapherConfig.selectedEntityColors ) // A sane join between years and days would create 5 days for the given input @@ -766,11 +801,9 @@ describe("creating a table from legacy", () => { ...getLegacyGrapherConfig(), selectedEntityColors: { "Cape Verde": "blue" }, } - const table = addSelectedEntityColorsToTable( - legacyToOwidTableAndDimensions( - getOwidVarSet(), - config.dimensions ?? [] - ), + const table = legacyToOwidTableAndDimensionsWithMandatorySlug( + getOwidVarSet(), + config.dimensions ?? [], config.selectedEntityColors ) @@ -783,20 +816,20 @@ describe("creating a table from legacy", () => { "entityName", "entityId", "entityCode", - "entityColor", "year", "3512", "time", // todo: what is the best design here? + "entityColor", ]) expect(table.columnNames).toEqual([ "Entity", "entityId", "Code", - "entityColor", "Year", name, "time", + "entityColor", ]) expect(table.get("3512").displayName).toBe("Some Display Name") @@ -806,9 +839,10 @@ describe("creating a table from legacy", () => { const varSet = getOwidVarSet() varSet.get(3512)!.metadata.display!.conversionFactor = 100 expect( - legacyToOwidTableAndDimensions( + legacyToOwidTableAndDimensionsWithMandatorySlug( varSet, - getLegacyGrapherConfig().dimensions ?? [] + getLegacyGrapherConfig().dimensions ?? [], + config.selectedEntityColors ).get("3512")!.values ).toEqual([550, 420, 1260]) }) @@ -829,9 +863,10 @@ Papua New Guinea,PNG,1983,5.5` it("passes on the non-redistributable flag", () => { const varSet = getOwidVarSet() varSet.get(3512)!.metadata.nonRedistributable = true - const columnDef = legacyToOwidTableAndDimensions( + const columnDef = legacyToOwidTableAndDimensionsWithMandatorySlug( varSet, - getLegacyGrapherConfig().dimensions ?? [] + getLegacyGrapherConfig().dimensions ?? [], + config.selectedEntityColors ).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 79b4fa25e8f..20836a136f1 100644 --- a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts +++ b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts @@ -9,6 +9,8 @@ import { OwidVariableDimensions, OwidVariableDataMetadataDimensions, EntityId, + ErrorValue, + OwidChartDimensionInterfaceWithMandatorySlug, } from "@ourworldindata/types" import { OwidTable, @@ -33,15 +35,18 @@ import { OwidVariableWithSourceAndDimension, ColumnSlug, EPOCH_DATE, - OwidChartDimensionInterface, OwidVariableType, memoize, + isEmpty, } from "@ourworldindata/utils" import { isContinentsVariableId } from "./GrapherConstants" export const legacyToOwidTableAndDimensions = ( json: MultipleOwidVariableDataDimensionsMap, - dimensions: OwidChartDimensionInterface[] + dimensions: OwidChartDimensionInterfaceWithMandatorySlug[], + selectedEntityColors: + | { [entityName: string]: string | undefined } + | undefined ): OwidTable => { // Entity meta map @@ -61,8 +66,7 @@ export const legacyToOwidTableAndDimensions = ( // We need to create a column for each unique [variable, targetTime] pair. So there can be // multiple columns for a single variable. - const newDimensions = computeActualDimensions(dimensions) - const dimensionColumns = uniqBy(newDimensions, (dim) => dim.slug) + const dimensionColumns = uniqBy(dimensions, (dim) => dim.slug) const variableTablesToJoinByYear: OwidTable[] = [] const variableTablesToJoinByDay: OwidTable[] = [] @@ -323,34 +327,36 @@ export const legacyToOwidTableAndDimensions = ( } } - return joinedVariablesTable -} - -export const addSelectedEntityColorsToTable = ( - table: OwidTable, - selectedEntityColors: { [entityName: string]: string | undefined } -): OwidTable => { - const entityColorColumnSlug = OwidTableSlugs.entityColor - - const valueFn = (entityId: EntityId | undefined) => { - if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString - const entityName = table.entityIdToNameMap.get(entityId) - return entityName && selectedEntityColors - ? (selectedEntityColors[entityName] ?? - ErrorValueTypes.UndefinedButShouldBeString) - : ErrorValueTypes.UndefinedButShouldBeString - } + // Append the entity color column if we have selected entity colors + if (!isEmpty(selectedEntityColors)) { + const entityColorColumnSlug = OwidTableSlugs.entityColor + + const valueFn = ( + entityId: EntityId | undefined + ): string | ErrorValue => { + if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString + const entityName = + joinedVariablesTable.entityIdToNameMap.get(entityId) + return entityName && selectedEntityColors + ? (selectedEntityColors[entityName] ?? + ErrorValueTypes.UndefinedButShouldBeString) + : ErrorValueTypes.UndefinedButShouldBeString + } - const values = table.rows.map((row) => valueFn(row.entityId)) + const values = joinedVariablesTable.rows.map((row) => + valueFn(row.entityId) + ) - return table.appendColumns([ - { - slug: entityColorColumnSlug, - name: entityColorColumnSlug, - type: ColumnTypeNames.Color, - values: values, - }, - ]) + joinedVariablesTable = joinedVariablesTable.appendColumns([ + { + slug: entityColorColumnSlug, + name: entityColorColumnSlug, + type: ColumnTypeNames.Color, + values: values, + }, + ]) + } + return joinedVariablesTable } const fullJoinTables = ( @@ -738,17 +744,6 @@ const annotationsToMap = (annotations: string): Map => { return entityAnnotationsMap } -export function computeActualDimensions( - dimensions: OwidChartDimensionInterface[] -): OwidChartDimensionInterface[] { - return dimensions.map((dimension) => ({ - ...dimension, - slug: dimension.targetYear - ? `${dimension.variableId}-${dimension.targetYear}` - : `${dimension.variableId}`, - })) -} - /** * Loads a single variable into an OwidTable. */ diff --git a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts index 6c7c0a182cd..a4cdf8d60c7 100644 --- a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts +++ b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts @@ -42,3 +42,8 @@ export interface OwidChartDimensionInterface { variableId: OwidVariableId slug?: ColumnSlug } + +export interface OwidChartDimensionInterfaceWithMandatorySlug + extends OwidChartDimensionInterface { + slug: ColumnSlug +} diff --git a/packages/@ourworldindata/types/src/index.ts b/packages/@ourworldindata/types/src/index.ts index 6ce5f408ed0..3a668ffb8ef 100644 --- a/packages/@ourworldindata/types/src/index.ts +++ b/packages/@ourworldindata/types/src/index.ts @@ -412,6 +412,7 @@ export { type OwidVariableDataTableConfigInterface, OwidVariableRoundingMode, type OwidChartDimensionInterface, + type OwidChartDimensionInterfaceWithMandatorySlug, } from "./OwidVariableDisplayConfigInterface.js" export { From d1488269976867104c5413b17b08aa48103a157a Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Thu, 30 Jan 2025 22:28:40 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=94=A8=20incorporate=20PR=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/core/LegacyToOwidTable.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts index 20836a136f1..cde6503843f 100644 --- a/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts +++ b/packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts @@ -8,9 +8,9 @@ import { OwidColumnDef, OwidVariableDimensions, OwidVariableDataMetadataDimensions, - EntityId, ErrorValue, OwidChartDimensionInterfaceWithMandatorySlug, + EntityName, } from "@ourworldindata/types" import { OwidTable, @@ -332,20 +332,19 @@ export const legacyToOwidTableAndDimensions = ( const entityColorColumnSlug = OwidTableSlugs.entityColor const valueFn = ( - entityId: EntityId | undefined + entityName: EntityName | undefined ): string | ErrorValue => { - if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString - const entityName = - joinedVariablesTable.entityIdToNameMap.get(entityId) + if (!entityName) return ErrorValueTypes.UndefinedButShouldBeString return entityName && selectedEntityColors ? (selectedEntityColors[entityName] ?? ErrorValueTypes.UndefinedButShouldBeString) : ErrorValueTypes.UndefinedButShouldBeString } - const values = joinedVariablesTable.rows.map((row) => - valueFn(row.entityId) - ) + const values = + joinedVariablesTable.entityNameColumn.valuesIncludingErrorValues.map( + (entityName) => valueFn(entityName as EntityName) + ) joinedVariablesTable = joinedVariablesTable.appendColumns([ { From a023ee0c6c1e73cfd4a96ce0cc8b2687c95ef144 Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Mon, 3 Feb 2025 09:18:31 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=94=A8=20Set=20dimensions=20again=20a?= =?UTF-8?q?fter=20loading=20the=20input=20table=20(tests=20and=20svg=20tes?= =?UTF-8?q?ter=20were=20green=20without=20it,=20probably=20not=20necessary?= =?UTF-8?q?=20and=20will=20be=20replaced=20in=20a=20later=20PR)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../grapher/src/core/Grapher.tsx | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 994a25f8b23..222b77b38e1 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -114,6 +114,7 @@ import { GrapherTabOption, SeriesName, ChartViewInfo, + OwidChartDimensionInterfaceWithMandatorySlug, } from "@ourworldindata/types" import { BlankOwidTable, @@ -1180,18 +1181,19 @@ export class Grapher // TODO grapher model: switch this to downloading multiple data and metadata files const startMark = performance.now() - const dimensions = legacyConfig.dimensions?.map((dimension) => ({ - ...dimension, - slug: - dimension.slug ?? - getDimensionColumnSlug( - dimension.variableId, - dimension.targetYear - ), - })) + const dimensions: OwidChartDimensionInterfaceWithMandatorySlug[] = + legacyConfig.dimensions?.map((dimension) => ({ + ...dimension, + slug: + dimension.slug ?? + getDimensionColumnSlug( + dimension.variableId, + dimension.targetYear + ), + })) ?? [] const tableWithColors = legacyToOwidTableAndDimensions( json, - dimensions ?? [], + dimensions, legacyConfig.selectedEntityColors ) this.createPerformanceMeasurement( @@ -1203,6 +1205,10 @@ export class Grapher this.inputTable = inputTableTransformer(tableWithColors) else this.inputTable = tableWithColors + // We need to reset the dimensions because some of them may have changed slugs in the legacy + // transformation (can happen when columns use targetTime) + this.setDimensionsFromConfigs(dimensions) + this.appendNewEntitySelectionOptions() if (this.manager?.selection?.hasSelection) {