search

owid / owid-grapher

A platform for creating interactive data visualizations
https://ourworldindata.org
MIT License
1.38k stars 229 forks source link

Faceting: Relative mode wrong for "Split by metric" mode #2136

Open marcelgerber opened 1 year ago

marcelgerber commented 1 year ago

Steps to reproduce

Steps to reproduce the behavior:

  1. Go to https://ourworldindata.org/grapher/urban-and-rural-population-2050?stackMode=relative&facet=metric&country=OWID_WRL~DEU
  2. Take a deep breath

Screenshots

image

It is, of course, not the case that half the world's urban population lives in Germany.

Additional context

Sigh.

What's going on here is that the data flow is super duper weird.

https://github.com/owid/owid-grapher/blob/9ad70c7b365c7722c0626065da61a6099b953187/packages/%40ourworldindata/grapher/src/stackedCharts/AbstractStackedChart.tsx#L73-L79

This code is called twice, first with manager = Grapher and an auto-detected seriesStrategy = column. The call to toPercentageFromEachColumnForEachEntityAndTime then makes it so Urban + Rural = 100% for each country.

Then, later on, this code is called again, with the table from after the above code has run (😱). Now the manager is provided by FacetChart with an explicit seriesStrategy = entity set, and so this time toPercentageFromEachEntityForEachTime runs, which makes it so World + Germany = 100% for Urban, Rural.

So, in effect, we're running both relative transforms in sequence, which is absolutely wrong and should never happen. The problem, I guess, is that Grapher.tsx instantiates a StackedArea chart already that isn't faceted at all, and runs transforms on that one already.

This is gonna be tough to fix. It sure would be nice to fix this, because this can cause some super weird issues with other chart types too I'm sure. But if we don't end up wanting to tackle this, then we should probably turn of relative mode for "Split by metric" mode.

sophiamersmann commented 1 year ago

Now that faceting is enabled for more charts I think this impacts ~200 charts. Since this is a pretty serious bug and we can't quickly fix this, maybe we really should think about turning off relative mode for "Split by metric" for now?

sophiamersmann commented 1 year ago

Are you positive this affects "Split by metric" only or might "Split by entity" also be affected, @marcelgerber?

marcelgerber commented 1 year ago

I'm 99% certain this only affects "Split by metric".

sophiamersmann commented 1 year ago

haha 99% is good enough for me 😄 Thanks!

sophiamersmann commented 1 year ago

2176 implements a hot fix and turns off relative mode for stacked area/bar charts that are split by metric. This is meant to be temporary. Once this issue is properly addressed, this code can be removed. Check out the diff here: https://github.com/owid/owid-grapher/pull/2176/files

larsyencken commented 1 year ago

Since we don't know how deep the rabbit hole goes, we should probably time box an initial investigation into this to 3 days and then pop back up for air and work out again how big we think it is then.

danyx23 commented 7 months ago

It seems harder to reach this case now so we are downgrading it to nice to have

danyx23 commented 7 months ago

One option would also be to disallow "split by metric" for stacked area charts