From 93cd99a872bd1da4021a1cf7772ce7759affc9d0 Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Wed, 11 Dec 2024 13:51:29 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=89=20(slope)=20add=20tolerance=20/=20?= =?UTF-8?q?TAS-719=20(#4205)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds tolerance to slope charts. If the original times of start and end value are too close together, then we don't show the slope. This is something that came out of discussion with Joe and Daniel, but now that I'm working on this, I don't think we need to be that strict? As long as the original start time < the original end time, we should be good? Curious what you think! ### Testing Example charts: - http://staging-site-slope-tolerance-viz/grapher/ratio-of-female-to-male-labor-force-participation-rates-slopes?time=2000..latest&country=AGO~OWID_WRL - With forced relative mode (doesn't make much sense here, just for testing): http://staging-site-slope-tolerance-viz/grapher/ratio-of-female-to-male-labor-force-participation-rates-slopes?time=2000..latest&country=AGO~OWID_WRL&stackMode=relative --- .../core-table/src/CoreTableColumns.ts | 25 +++- .../core-table/src/OwidTable.test.ts | 8 +- .../grapher/src/core/Grapher.tsx | 5 +- .../src/entitySelector/EntitySelector.tsx | 2 +- .../grapher/src/mapCharts/MapChart.tsx | 4 +- .../grapher/src/mapCharts/MapSparkline.tsx | 2 +- .../grapher/src/mapCharts/MapTooltip.tsx | 6 +- .../src/slopeCharts/SlopeChart.test.ts | 3 +- .../grapher/src/slopeCharts/SlopeChart.tsx | 126 ++++++++++++++---- .../src/slopeCharts/SlopeChartConstants.ts | 12 +- .../stackedCharts/AbstractStackedChart.tsx | 4 +- .../src/stackedCharts/MarimekkoChart.tsx | 4 +- .../stackedCharts/StackedDiscreteBarChart.tsx | 2 +- .../grapher/src/tooltip/TooltipContents.tsx | 8 +- .../types/src/domainTypes/CoreTableTypes.ts | 1 + 15 files changed, 154 insertions(+), 58 deletions(-) diff --git a/packages/@ourworldindata/core-table/src/CoreTableColumns.ts b/packages/@ourworldindata/core-table/src/CoreTableColumns.ts index 390dfb88bcd..64b355d867e 100644 --- a/packages/@ourworldindata/core-table/src/CoreTableColumns.ts +++ b/packages/@ourworldindata/core-table/src/CoreTableColumns.ts @@ -536,14 +536,16 @@ export abstract class AbstractCoreColumn { // assumes table is sorted by time @imemo get owidRows(): OwidVariableRow[] { const entities = this.allEntityNames - const times = this.originalTimes + const times = this.allTimes const values = this.values + const originalTimes = this.originalTimes const originalValues = this.originalValues - return range(0, times.length).map((index) => { + return range(0, originalTimes.length).map((index) => { return omitUndefinedValues({ entityName: entities[index], time: times[index], value: values[index], + originalTime: originalTimes[index], originalValue: originalValues[index], }) }) @@ -562,6 +564,23 @@ export abstract class AbstractCoreColumn { return map } + // todo: remove? Should not be on CoreTable + @imemo get owidRowByEntityNameAndTime(): Map< + EntityName, + Map> + > { + const valueByEntityNameAndTime = new Map< + EntityName, + Map> + >() + this.owidRows.forEach((row) => { + if (!valueByEntityNameAndTime.has(row.entityName)) + valueByEntityNameAndTime.set(row.entityName, new Map()) + valueByEntityNameAndTime.get(row.entityName)!.set(row.time, row) + }) + return valueByEntityNameAndTime + } + // todo: remove? Should not be on CoreTable // NOTE: this uses the original times, so any tolerance is effectively unapplied. @imemo get valueByEntityNameAndOriginalTime(): Map< @@ -577,7 +596,7 @@ export abstract class AbstractCoreColumn { valueByEntityNameAndTime.set(row.entityName, new Map()) valueByEntityNameAndTime .get(row.entityName)! - .set(row.time, row.value) + .set(row.originalTime, row.value) }) return valueByEntityNameAndTime } diff --git a/packages/@ourworldindata/core-table/src/OwidTable.test.ts b/packages/@ourworldindata/core-table/src/OwidTable.test.ts index a58250a1ec7..32208310965 100755 --- a/packages/@ourworldindata/core-table/src/OwidTable.test.ts +++ b/packages/@ourworldindata/core-table/src/OwidTable.test.ts @@ -72,7 +72,7 @@ it("can parse data to Javascript data structures", () => { table.get("Population").owidRows.forEach((row) => { expect(typeof row.entityName).toBe("string") expect(row.value).toBeGreaterThan(100) - expect(row.time).toBeGreaterThan(1999) + expect(row.originalTime).toBeGreaterThan(1999) }) }) @@ -632,7 +632,7 @@ describe("tolerance", () => { }) }) -it("assigns originalTime as 'time' in owidRows", () => { +it("assigns originalTime as 'originalTime' in owidRows", () => { const csv = `gdp,year,entityName,entityId,entityCode 1000,2019,USA,, 1001,2020,UK,,` @@ -642,7 +642,7 @@ it("assigns originalTime as 'time' in owidRows", () => { expect.not.arrayContaining([ expect.objectContaining({ entityName: "USA", - time: 2020, + originalTime: 2020, value: 1000, }), ]) @@ -651,7 +651,7 @@ it("assigns originalTime as 'time' in owidRows", () => { expect.not.arrayContaining([ expect.objectContaining({ entityName: "UK", - time: 2019, + originalTime: 2019, value: 1001, }), ]) diff --git a/packages/@ourworldindata/grapher/src/core/Grapher.tsx b/packages/@ourworldindata/grapher/src/core/Grapher.tsx index 673b56b3167..e85e5f21070 100644 --- a/packages/@ourworldindata/grapher/src/core/Grapher.tsx +++ b/packages/@ourworldindata/grapher/src/core/Grapher.tsx @@ -884,7 +884,10 @@ export class Grapher ) if (this.isOnSlopeChartTab) - return table.filterByTargetTimes([startTime, endTime]) + return table.filterByTargetTimes( + [startTime, endTime], + table.get(this.yColumnSlugs[0]).tolerance + ) return table.filterByTimeRange(startTime, endTime) } diff --git a/packages/@ourworldindata/grapher/src/entitySelector/EntitySelector.tsx b/packages/@ourworldindata/grapher/src/entitySelector/EntitySelector.tsx index ebce9d92057..a6590dcf0be 100644 --- a/packages/@ourworldindata/grapher/src/entitySelector/EntitySelector.tsx +++ b/packages/@ourworldindata/grapher/src/entitySelector/EntitySelector.tsx @@ -453,7 +453,7 @@ export class EntitySelector extends React.Component<{ const rows = column.owidRowsByEntityName.get(entityName) ?? [] searchableEntity[column.slug] = maxBy( rows, - (row) => row.time + (row) => row.originalTime )?.value } diff --git a/packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx b/packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx index f641a1b939b..a07da5438fa 100644 --- a/packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx +++ b/packages/@ourworldindata/grapher/src/mapCharts/MapChart.tsx @@ -346,12 +346,12 @@ export class MapChart return mapColumn.owidRows .map((row) => { - const { entityName, value, time } = row + const { entityName, value, originalTime } = row const color = this.colorScale.getColor(value) || "red" // todo: color fix if (!color) return undefined return { seriesName: entityName, - time, + time: originalTime, value, isSelected: selectionArray.selectedSet.has(entityName), color, diff --git a/packages/@ourworldindata/grapher/src/mapCharts/MapSparkline.tsx b/packages/@ourworldindata/grapher/src/mapCharts/MapSparkline.tsx index 7fa2a61dbfb..72977920cdb 100644 --- a/packages/@ourworldindata/grapher/src/mapCharts/MapSparkline.tsx +++ b/packages/@ourworldindata/grapher/src/mapCharts/MapSparkline.tsx @@ -118,7 +118,7 @@ export class MapSparkline extends React.Component<{ lineStrokeWidth: 2, entityYearHighlight: { entityName: this.manager.entityName, - year: this.manager.datum?.time, + year: this.manager.datum?.originalTime, }, yAxisConfig: { hideAxis: true, diff --git a/packages/@ourworldindata/grapher/src/mapCharts/MapTooltip.tsx b/packages/@ourworldindata/grapher/src/mapCharts/MapTooltip.tsx index e58b90c5484..65775d51db1 100644 --- a/packages/@ourworldindata/grapher/src/mapCharts/MapTooltip.tsx +++ b/packages/@ourworldindata/grapher/src/mapCharts/MapTooltip.tsx @@ -119,8 +119,8 @@ export class MapTooltip : targetTime?.toString() const displayDatumTime = timeColumn && datum - ? timeColumn.formatValue(datum?.time) - : (datum?.time.toString() ?? "") + ? timeColumn.formatValue(datum?.originalTime) + : (datum?.originalTime.toString() ?? "") const valueColor: string | undefined = darkenColorForHighContrastText( lineColorScale?.getColor(datum?.value) ?? "#333" ) @@ -143,7 +143,7 @@ export class MapTooltip const yColumn = this.mapTable.get(this.mapColumnSlug) const targetNotice = - datum && datum.time !== targetTime ? displayTime : undefined + datum && datum.originalTime !== targetTime ? displayTime : undefined const toleranceNotice = targetNotice ? { icon: TooltipFooterIcon.notice, diff --git a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.test.ts b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.test.ts index bdfb7d1885c..8fdabb7dd4e 100755 --- a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.test.ts +++ b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.test.ts @@ -49,7 +49,8 @@ it("filters non-numeric values", () => { expect(chart.series.length).toEqual(1) expect( chart.series.every( - (series) => isNumber(series.startValue) && isNumber(series.endValue) + (series) => + isNumber(series.start.value) && isNumber(series.end.value) ) ).toBeTruthy() }) diff --git a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx index 8e928278f04..4f2130bf8db 100644 --- a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx +++ b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx @@ -60,6 +60,7 @@ import { ColorSchemes } from "../color/ColorSchemes" import { LineLabelSeries, LineLegend } from "../lineLegend/LineLegend" import { makeTooltipRoundingNotice, + makeTooltipToleranceNotice, Tooltip, TooltipState, TooltipValueRange, @@ -114,12 +115,20 @@ export class SlopeChart if (this.isLogScale) table = table.replaceNonPositiveCellsForLogScale(this.yColumnSlugs) + this.yColumnSlugs.forEach((slug) => { + table = table.interpolateColumnWithTolerance(slug) + }) + return table } transformTableForSelection(table: OwidTable): OwidTable { table = table.replaceNonNumericCellsWithErrorValues(this.yColumnSlugs) + this.yColumnSlugs.forEach((slug) => { + table = table.interpolateColumnWithTolerance(slug) + }) + // if time selection is disabled, then filter all entities that // don't have data for the current time period if (!this.manager.hasTimeline && this.startTime !== this.endTime) { @@ -276,10 +285,9 @@ export class SlopeChart canSelectMultipleEntities, }) - const valueByTime = - column.valueByEntityNameAndOriginalTime.get(entityName) - const startValue = valueByTime?.get(startTime) - const endValue = valueByTime?.get(endTime) + const owidRowByTime = column.owidRowByEntityNameAndTime.get(entityName) + const start = owidRowByTime?.get(startTime) + const end = owidRowByTime?.get(endTime) const colorKey = getColorKey({ entityName, @@ -299,8 +307,8 @@ export class SlopeChart seriesName, entityName, color, - startValue, - endValue, + start, + end, annotation, } } @@ -308,7 +316,29 @@ export class SlopeChart private isSeriesValid( series: RawSlopeChartSeries ): series is SlopeChartSeries { - return series.startValue !== undefined && series.endValue !== undefined + const { + start, + end, + column: { tolerance }, + } = series + + // if the start or end value is missing, we can't draw the slope + if (start?.value === undefined || end?.value === undefined) return false + + // sanity check (might happen if tolerance is enabled) + if (start.originalTime >= end.originalTime) return false + + const isToleranceAppliedToStartValue = + start.originalTime !== this.startTime + const isToleranceAppliedToEndValue = end.originalTime !== this.endTime + + // if tolerance has been applied to one of the values, then we require + // a minimal distance between the original times + if (isToleranceAppliedToStartValue || isToleranceAppliedToEndValue) { + return end.originalTime - start.originalTime >= tolerance + } + + return true } // Usually we drop rows with missing data in the transformTable function. @@ -370,8 +400,8 @@ export class SlopeChart const { yAxis, startX, endX } = this return this.series.map((series) => { - const startY = yAxis.place(series.startValue) - const endY = yAxis.place(series.endValue) + const startY = yAxis.place(series.start.value) + const endY = yAxis.place(series.end.value) const startPoint = new PointVector(startX, startY) const endPoint = new PointVector(endX, endY) @@ -406,8 +436,8 @@ export class SlopeChart @computed get allYValues(): number[] { return this.series.flatMap((series) => [ - series.startValue, - series.endValue, + series.start.value, + series.end.value, ]) } @@ -522,13 +552,13 @@ export class SlopeChart // used in LineLegend @computed get labelSeries(): LineLabelSeries[] { return this.series.map((series) => { - const { seriesName, color, endValue, annotation } = series + const { seriesName, color, end, annotation } = series return { color, seriesName, label: seriesName, annotation, - yValue: endValue, + yValue: end.value, } }) } @@ -660,17 +690,37 @@ export class SlopeChart const { series } = target || {} if (!series) return - const title = isRelativeMode - ? `${series.seriesName}, ${formatColumn.formatTime(endTime)}` - : series.seriesName + const formatTime = (time: Time) => formatColumn.formatTime(time) - const timeRange = [startTime, endTime] - .map((t) => formatColumn.formatTime(t)) - .join(" to ") + const actualStartTime = series.start.originalTime + const actualEndTime = series.end.originalTime + const timeRange = `${formatTime(actualStartTime)} to ${formatTime(actualEndTime)}` const timeLabel = isRelativeMode - ? `% change since ${formatColumn.formatTime(startTime)}` + ? `% change between ${formatColumn.formatTime(actualStartTime)} and ${formatColumn.formatTime(actualEndTime)}` : timeRange + const constructTargetYearForToleranceNotice = () => { + const isStartValueOriginal = series.start.originalTime === startTime + const isEndValueOriginal = series.end.originalTime === endTime + + if (!isStartValueOriginal && !isEndValueOriginal) { + return `${formatTime(startTime)} and ${formatTime(endTime)}` + } else if (!isStartValueOriginal) { + return formatTime(startTime) + } else if (!isEndValueOriginal) { + return formatTime(endTime) + } else { + return undefined + } + } + + const targetYear = constructTargetYearForToleranceNotice() + const toleranceNotice = targetYear + ? { + icon: TooltipFooterIcon.notice, + text: makeTooltipToleranceNotice(targetYear), + } + : undefined const roundingNotice = series.column.roundsToSignificantFigures ? { icon: TooltipFooterIcon.none, @@ -680,11 +730,11 @@ export class SlopeChart ), } : undefined - const footer = excludeUndefined([roundingNotice]) + const footer = excludeUndefined([toleranceNotice, roundingNotice]) const values = isRelativeMode - ? [series.endValue] - : [series.startValue, series.endValue] + ? [series.end.value] + : [series.start.value, series.end.value] return ( (this.tooltipState.target = null)} @@ -707,16 +758,35 @@ export class SlopeChart } private makeMissingDataLabel(series: RawSlopeChartSeries): string { - const { seriesName } = series + const { seriesName, start, end } = series + const startTime = this.formatColumn.formatTime(this.startTime) const endTime = this.formatColumn.formatTime(this.endTime) - if (series.startValue === undefined && series.endValue === undefined) { + + // mention the start or end value if they're missing + if (start?.value === undefined && end?.value === undefined) { return `${seriesName} (${startTime} & ${endTime})` - } else if (series.startValue === undefined) { + } else if (start?.value === undefined) { return `${seriesName} (${startTime})` - } else if (series.endValue === undefined) { + } else if (end?.value === undefined) { return `${seriesName} (${endTime})` } + + // if both values are given but the series shows up in the No Data + // section, then tolerance has been applied to one of the values + // in such a way that we decided not to render the slope after all + // (e.g. when the original times are too close to each other) + const isToleranceAppliedToStartValue = + start.originalTime !== this.startTime + const isToleranceAppliedToEndValue = end.originalTime !== this.endTime + if (isToleranceAppliedToStartValue && isToleranceAppliedToEndValue) { + return `${seriesName} (${startTime} & ${endTime})` + } else if (isToleranceAppliedToStartValue) { + return `${seriesName} (${startTime})` + } else if (isToleranceAppliedToEndValue) { + return `${seriesName} (${endTime})` + } + return seriesName } diff --git a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChartConstants.ts b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChartConstants.ts index 85cd862866e..a976bed8e2c 100644 --- a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChartConstants.ts +++ b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChartConstants.ts @@ -1,19 +1,17 @@ -import { EntityName, PartialBy, PointVector } from "@ourworldindata/utils" +import { PartialBy, PointVector } from "@ourworldindata/utils" +import { EntityName, OwidVariableRow } from "@ourworldindata/types" import { ChartSeries } from "../chart/ChartInterface" import { CoreColumn } from "@ourworldindata/core-table" export interface SlopeChartSeries extends ChartSeries { column: CoreColumn entityName: EntityName - startValue: number - endValue: number + start: Pick, "value" | "originalTime"> + end: Pick, "value" | "originalTime"> annotation?: string } -export type RawSlopeChartSeries = PartialBy< - SlopeChartSeries, - "startValue" | "endValue" -> +export type RawSlopeChartSeries = PartialBy export interface PlacedSlopeChartSeries extends SlopeChartSeries { startPoint: PointVector diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/AbstractStackedChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/AbstractStackedChart.tsx index 2a5110442d5..5b40a3086bb 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/AbstractStackedChart.tsx +++ b/packages/@ourworldindata/grapher/src/stackedCharts/AbstractStackedChart.tsx @@ -407,8 +407,8 @@ export class AbstractStackedChart const pointColor = row.value > 0 ? POSITIVE_COLOR : NEGATIVE_COLOR return { - position: row.time, - time: row.time, + position: row.originalTime, + time: row.originalTime, value: row.value, valueOffset: 0, interpolated: diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx index 9fa7f41afd6..e89b176b4da 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx +++ b/packages/@ourworldindata/grapher/src/stackedCharts/MarimekkoChart.tsx @@ -383,7 +383,7 @@ export class MarimekkoChart col.def.color ?? colorScheme.getColors(yColumns.length)[i], points: col.owidRows.map((row) => ({ - time: row.time, + time: row.originalTime, position: row.entityName, value: row.value, valueOffset: 0, @@ -417,7 +417,7 @@ export class MarimekkoChart const points: SimplePoint[] = [] for (const row of rows) { points.push({ - time: row.time, + time: row.originalTime, value: row.value, entity: row.entityName, }) diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/StackedDiscreteBarChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/StackedDiscreteBarChart.tsx index c575e65c476..b298d2ae160 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/StackedDiscreteBarChart.tsx +++ b/packages/@ourworldindata/grapher/src/stackedCharts/StackedDiscreteBarChart.tsx @@ -1006,7 +1006,7 @@ export class StackedDiscreteBarChart col.displayName ), points: col.owidRows.map((row) => ({ - time: row.time, + time: row.originalTime, position: row.entityName, value: row.value, valueOffset: 0, diff --git a/packages/@ourworldindata/grapher/src/tooltip/TooltipContents.tsx b/packages/@ourworldindata/grapher/src/tooltip/TooltipContents.tsx index 3078fd31d53..a3a8b7f0f1d 100644 --- a/packages/@ourworldindata/grapher/src/tooltip/TooltipContents.tsx +++ b/packages/@ourworldindata/grapher/src/tooltip/TooltipContents.tsx @@ -337,8 +337,12 @@ export function IconCircledS({ ) } -export function makeTooltipToleranceNotice(targetYear: string): string { - return `Data not available for ${targetYear}. Showing closest available data point instead` +export function makeTooltipToleranceNotice( + targetYear: string, + { plural }: { plural: boolean } = { plural: false } +): string { + const dataPoint = plural ? "data points" : "data point" + return `Data not available for ${targetYear}. Showing closest available ${dataPoint} instead` } export function makeTooltipRoundingNotice( diff --git a/packages/@ourworldindata/types/src/domainTypes/CoreTableTypes.ts b/packages/@ourworldindata/types/src/domainTypes/CoreTableTypes.ts index 5fa5ba92b00..a44d6d42459 100644 --- a/packages/@ourworldindata/types/src/domainTypes/CoreTableTypes.ts +++ b/packages/@ourworldindata/types/src/domainTypes/CoreTableTypes.ts @@ -300,5 +300,6 @@ export interface OwidVariableRow { entityName: EntityName time: Time value: ValueType + originalTime: Time originalValue?: ValueType }