diff --git a/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts b/packages/@ourworldindata/grapher/src/chart/ChartDimension.ts index 370cc10327..c5af906e29 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 8cb3ef69cd..08632a43db 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -140,6 +140,7 @@ import { loadVariableDataAndMetadata } from "./loadVariable" import Cookies from "js-cookie" import { ChartDimension, + getDimensionColumnSlug, LegacyDimensionsManager, } from "../chart/ChartDimension" import { TooltipManager } from "../tooltip/TooltipProps" @@ -191,11 +192,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, @@ -1151,19 +1148,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 ?? [] + dimensions ?? [], + legacyConfig.selectedEntityColors ) - const tableWithColors = legacyConfig.selectedEntityColors - ? addSelectedEntityColorsToTable( - table, - legacyConfig.selectedEntityColors - ) - : table - const dimensions = legacyConfig.dimensions - ? computeActualDimensions(legacyConfig.dimensions) - : [] this.createPerformanceMeasurement( "legacyToOwidTableAndDimensions", startMark @@ -1173,10 +1171,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 2766a107bb..a6bd3265a0 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 79b4fa25e8..20836a136f 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 6c7c0a182c..a4cdf8d60c 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 f5fd897166..0311df7b93 100644 --- a/packages/@ourworldindata/types/src/index.ts +++ b/packages/@ourworldindata/types/src/index.ts @@ -408,6 +408,7 @@ export { type OwidVariableDataTableConfigInterface, OwidVariableRoundingMode, type OwidChartDimensionInterface, + type OwidChartDimensionInterfaceWithMandatorySlug, } from "./OwidVariableDisplayConfigInterface.js" export {