Skip to content

Commit

Permalink
🔨 simplify working with the new legacyToOwidTable
Browse files Browse the repository at this point in the history
  • Loading branch information
@danyx23
danyx23 committed Jan 13, 2025
1 parent 0cfc5f9 commit 1f8d7f2
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 101 deletions.
22 changes: 20 additions & 2 deletions packages/@ourworldindata/grapher/src/chart/ChartDimension.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
OwidVariableDisplayConfig,
OwidChartDimensionInterface,
Time,
OwidChartDimensionInterfaceWithMandatorySlug,
} from "@ourworldindata/utils"
import { OwidTable, CoreColumn } from "@ourworldindata/core-table"

Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down
34 changes: 14 additions & 20 deletions packages/@ourworldindata/grapher/src/core/Grapher.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down
111 changes: 73 additions & 38 deletions packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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'", () => {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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'", () => {
Expand All @@ -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
Expand Down Expand Up @@ -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
)

Expand All @@ -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")
Expand All @@ -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])
})
Expand All @@ -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)
})
Expand Down
Loading

0 comments on commit 1f8d7f2

Please sign in to comment.