Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor input table #4375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+160 −91
Open

Refactor input table #4375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cfc5f9
🔨 make sure to add selected entity colors
danyx23 Dec 23, 2024
1f8d7f2
🔨 simplify working with the new legacyToOwidTable
danyx23 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions packages/@ourworldindata/grapher/src/chart/ChartDimension.ts
View file
Open in desktop
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
23 changes: 15 additions & 8 deletions packages/@ourworldindata/grapher/src/core/Grapher.tsx
View file
Open in desktop
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 @@ -1147,22 +1148,28 @@ export class Grapher
// TODO grapher model: switch this to downloading multiple data and metadata files

const startMark = performance.now()
const { dimensions, table } = legacyToOwidTableAndDimensions(
const dimensions = legacyConfig.dimensions?.map((dimension) => ({
...dimension,
slug:
dimension.slug ??
getDimensionColumnSlug(
dimension.variableId,
dimension.targetYear
),
}))
const tableWithColors = legacyToOwidTableAndDimensions(
json,
legacyConfig
dimensions ?? [],
legacyConfig.selectedEntityColors
)
this.createPerformanceMeasurement(
"legacyToOwidTableAndDimensions",
startMark
)

if (inputTableTransformer)
this.inputTable = inputTableTransformer(table)
else this.inputTable = table

// 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)
Comment on lines -1163 to -1165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you positive we don't need this call any more? I think it makes sense to keep it around, since dimensions can still change during this call, and therefore this.dimensions may drift out of sync?

this.inputTable = inputTableTransformer(tableWithColors)
else this.inputTable = tableWithColors

this.appendNewEntitySelectionOptions()

Expand Down
115 changes: 78 additions & 37 deletions packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,40 @@ import {
OwidTableSlugs,
StandardOwidColumnDefs,
LegacyGrapherInterface,
OwidChartDimensionInterface,
} from "@ourworldindata/types"
import { ColumnTypeMap, ErrorValueTypes } from "@ourworldindata/core-table"
import {
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 @@ -43,9 +69,10 @@ describe(legacyToOwidTableAndDimensions, () => {
}

it("contains the standard entity columns", () => {
const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
legacyGrapherConfig
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)
expect(table.columnSlugs).toEqual(
expect.arrayContaining(
Expand All @@ -62,27 +89,27 @@ describe(legacyToOwidTableAndDimensions, () => {

describe("conversionFactor", () => {
it("applies the more specific chart-level conversionFactor", () => {
const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
{
dimensions: [
{
variableId: 2,
display: { conversionFactor: 10 },
property: DimensionProperty.y,
},
],
}
[
{
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
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

// Apply the variable-level conversionFactor (100)
Expand Down Expand Up @@ -172,9 +199,10 @@ describe(legacyToOwidTableAndDimensions, () => {
],
}

const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
legacyGrapherConfig
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

it("leaves ErrorValues when there were no values to join to", () => {
Expand Down Expand Up @@ -315,9 +343,10 @@ describe(legacyToOwidTableAndDimensions, () => {
],
}

const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
legacyGrapherConfig
legacyGrapherConfig.dimensions ?? [],
{}
)

it("shifts values in days array when zeroDay is is not EPOCH_DATE", () => {
Expand Down Expand Up @@ -438,9 +467,10 @@ describe(legacyToOwidTableAndDimensions, () => {
],
}

const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
legacyGrapherConfig
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

it("duplicates 'day' column into 'time'", () => {
Expand Down Expand Up @@ -469,9 +499,10 @@ describe(legacyToOwidTableAndDimensions, () => {
chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
}

const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
scatterLegacyGrapherConfig
scatterLegacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

expect(table.rows.length).toEqual(3)
Expand Down Expand Up @@ -627,9 +658,10 @@ describe("variables with mixed days & years with missing overlap and multiple po
],
}

const { table } = legacyToOwidTableAndDimensions(
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
legacyVariableConfig,
legacyGrapherConfig
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

it("duplicates 'day' column into 'time'", () => {
Expand All @@ -646,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
legacyGrapherConfig.dimensions ?? [],
legacyGrapherConfig.selectedEntityColors
)

// A sane join between years and days would create 5 days for the given input
Expand Down Expand Up @@ -764,10 +797,16 @@ const getLegacyGrapherConfig = (): Partial<LegacyGrapherInterface> => {
}

describe("creating a table from legacy", () => {
const table = legacyToOwidTableAndDimensions(getOwidVarSet(), {
const config = {
...getLegacyGrapherConfig(),
selectedEntityColors: { "Cape Verde": "blue" },
}).table
}
const table = legacyToOwidTableAndDimensionsWithMandatorySlug(
getOwidVarSet(),
config.dimensions ?? [],
config.selectedEntityColors
)

const name =
"Prevalence of wasting, weight for height (% of children under 5)"

Expand All @@ -777,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 @@ -800,10 +839,11 @@ describe("creating a table from legacy", () => {
const varSet = getOwidVarSet()
varSet.get(3512)!.metadata.display!.conversionFactor = 100
expect(
legacyToOwidTableAndDimensions(
legacyToOwidTableAndDimensionsWithMandatorySlug(
varSet,
getLegacyGrapherConfig()
).table.get("3512")!.values
getLegacyGrapherConfig().dimensions ?? [],
config.selectedEntityColors
).get("3512")!.values
).toEqual([550, 420, 1260])
})

Expand All @@ -823,10 +863,11 @@ 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()
).table.get("3512").def as OwidColumnDef
getLegacyGrapherConfig().dimensions ?? [],
config.selectedEntityColors
).get("3512").def as OwidColumnDef
expect(columnDef.nonRedistributable).toEqual(true)
})
})
Loading
Loading