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

Jump to bottom

Refactor input table #4375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor input table #4375

wants to merge 2 commits into from
+160 −91

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Jan 3, 2025 •
edited
Loading

Refactor the creation of inputtable in anticipation of extracting the loading functionality. This PR does the following things:

  • legacyToOwidTable is changed to no longer require a full grapher config but only a dimensions array and the selected entity colors
  • dimensions is no longer returned from legacyToOwidTableAndDimensions - instead the slug adjustements are made as a getter in ChartDimension

@danyx23 danyx23 changed the base branch from master to christmas-features January 3, 2025 10:59
@danyx23 danyx23 force-pushed the christmas-features branch from 220031f to aea1c76 Compare January 3, 2025 12:10
@danyx23 danyx23 force-pushed the refactor-input-table branch 2 times, most recently from 5b465ed to e79f776 Compare January 6, 2025 14:18
@danyx23 danyx23 force-pushed the christmas-features branch 2 times, most recently from 4a1e753 to 5c47981 Compare January 9, 2025 10:37
@danyx23 danyx23 force-pushed the refactor-input-table branch from e79f776 to e4a7794 Compare January 9, 2025 10:39
@danyx23 danyx23 force-pushed the christmas-features branch from 5c47981 to cdde7f4 Compare January 9, 2025 20:25
@danyx23 danyx23 force-pushed the refactor-input-table branch from e4a7794 to 75a6fe6 Compare January 9, 2025 20:25
@danyx23 danyx23 force-pushed the christmas-features branch from cdde7f4 to ff2359c Compare January 10, 2025 08:46
@danyx23 danyx23 force-pushed the refactor-input-table branch 2 times, most recently from c6f2a9c to aea678b Compare January 10, 2025 17:50
@danyx23 danyx23 marked this pull request as ready for review January 10, 2025 17:57
@owidbot
Copy link
Contributor

owidbot commented Jan 10, 2025 •
edited
Loading

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-refactor-input-table

SVG tester:

Number of differences (default views): 1617 (24cd01) ❌
Number of differences (all views): 1020 (70b11c) ❌

Edited: 2025-01-10 18:08:15 UTC
Execution time: 1.29 seconds

@danyx23 danyx23 requested a review from marcelgerber January 10, 2025 17:58
marcelgerber
marcelgerber approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

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

Nice work!
I had some feedback, but otherwise this looks good.

For peace of mind, it would be nice if we can explicitly run the SVG tester on this set of changes (which it doesn't cleanly do here, because it's not on top of master).
Could you do that, potentially by opening a draft PR of this-PR-rebased-onto-master?

packages/@ourworldindata/grapher/src/core/Grapher.tsx
Comment on lines -1174 to -1165
// 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)
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?

packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts
Comment on lines +334 to +344
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
}
Copy link
Member

Choose a reason for hiding this comment

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

It is super odd that this fn is now based on EntityId, when it wasn't before.
Is there a particular reason for this? We have mostly deprecated entityId (as in, it is not really used within grapher code), and it is not present in explorer tables IIRC.
Could you instead use EntityName instead, unless there's a good reason not to?

packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts
Comment on lines +346 to +348
const values = joinedVariablesTable.rows.map((row) =>
valueFn(row.entityId)
)
Copy link
Member

Choose a reason for hiding this comment

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

perf: computing .rows is nontrivial (since our column store is column-based), and we should avoid it if we can.

you can instead use something like

joinedVariablesTable.entityNameColumn.valuesIncludingErrorValues.map()

@danyx23 danyx23 force-pushed the refactor-input-table branch from aea678b to 1f8d7f2 Compare January 13, 2025 14:11
@danyx23 danyx23 changed the base branch from christmas-features to master January 13, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelgerber marcelgerber marcelgerber approved these changes

Assignees

@danyx23 danyx23

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danyx23 @owidbot @marcelgerber