Fix column scaling ("Multiply by a number") bugs #43668

JesseSDevaney · 2024-06-05T16:28:33Z

Issues Fixed

Closes: Manual Y axis min/max don't take the "Multiply by" factor into account #12214
Closes: Using scaling on a question with multiple y-axis columns breaks tool-tips and y-axis label formatting #43536

Manual Y axis min/max don't take the "Multiply by" factor into account (#12214)

Demo

2024-06-04.17-18-46.mp4

How to Verify

Create a line chart with column scaling added to the metric/y-axis column
Turn on "Linear" y-axis scale
Turn off `"Auto y-axis range"
Input a custom range that is outside the bounds of the current min/max
- customMin < minimum dataset value
- customMax > maximum dataset value
See that the exact values you inputted are selected
Go back to step 1->6 and verify with:
- Different y-axis scales: ["Log", "Pow"]
- Different chart types: ["Bar", "Area", "Combo", "Scatter", "Waterfall", "Stacked Bar", "Stacked Area"]

Acceptance Criteria

Column scaling/multiply by factor should still work as expected
The custom y-axis range given should be used as is and should not have to be adjusted based on data transformations - y-axis scale type (linear, pow, log) and metric value scaling
- This behavior should be extended to all relevant charts (line, area, bar, scatter, waterfall, stacked/non-stacked)
Loki variant added to verify and enforce the expected behavior

Using scaling on a question with multiple y-axis columns breaks tool-tips and y-axis label formatting (#43536)

Demo

2024-06-04.17-24-28.mp4

How to Verify

Create a line chart with multiple aggregations (["Sum of Total", "Sum of Subtotal"]) and one breakout ("Created at: Year")
Toggle on data labels: "Show values for data points"
Set both metrics/y-axis columns selected to use the left y-axis.
Add column scaling to the either one of the metrics (but do not both with the same value)
See that the charts are plotted and behave as expected.
- All of the data point values line up with the correct corresponding y-axis tick value
- All of the data point values behave as expected: i.e. are displaying the correct column scaled value
Update the chart type to be stacked bar/stacked area
Turn the data labels on for the inside stacked segments as well ("Both")
See that the charts inside labels and bar sizes relative to their values behave as expected.
Hover over one of the stacks and verify that the tool-tip contains the correct values for both series (correct values are the correctly scaled values based off of each column's own scale factor)

Acceptance Criteria

Using column scaling with multiple y-axis metrics should generate the chart as expected
- If one column uses scaling and the other does not, we should still see that each of the plotted data points line up with the correct corresponding y-axis tick value
E2E test has been added to verify and enforce the expected behavior

github-actions · 2024-06-05T16:29:52Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 8659846...8888d47.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/components/ChartTooltip/KeyValuePairChartTooltip/KeyValuePairChartTooltip.tsx frontend/src/metabase/visualizations/components/ChartTooltip/utils.ts frontend/src/metabase/visualizations/echarts/cartesian/model/axis.ts frontend/src/metabase/visualizations/echarts/cartesian/model/dataset.ts frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts frontend/src/metabase/visualizations/echarts/cartesian/model/series.ts frontend/src/metabase/visualizations/echarts/cartesian/model/util.ts frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts frontend/src/metabase/visualizations/echarts/cartesian/scatter/model/index.ts frontend/src/metabase/visualizations/echarts/cartesian/waterfall/model/dataset.ts frontend/src/metabase/visualizations/echarts/cartesian/waterfall/model/index.ts frontend/src/metabase/visualizations/lib/tooltip.ts frontend/src/metabase/visualizations/shared/utils/data.ts frontend/src/metabase/visualizations/shared/utils/data.unit.spec.ts frontend/src/metabase/visualizations/types/hover.ts frontend/src/metabase/visualizations/visualizations/CartesianChart/events.ts frontend/src/metabase/visualizations/visualizations/RowChart/RowChart.tsx frontend/src/metabase/visualizations/visualizations/RowChart/utils/format.ts

replay-io · 2024-06-05T16:40:20Z

Status	Complete ↗︎
Commit	`8888d47`
Results	⚠️ 4 Flaky ✅ 2577 Passed

…bels

…for formatters

…ataset

EmmadUsmani

Looks good! Could you add some more specs similar to the line chart one you created before merging? Ideally one for the scatter plot and waterfall chart would be nice, since they use different codepaths

frontend/src/metabase/visualizations/echarts/cartesian/model/series.ts

frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts

JesseSDevaney · 2024-06-05T17:56:46Z

frontend/src/metabase/static-viz/lib/format.ts

+  const options = getFormattingOptionsWithoutScaling({
+    column,
+    ...columnSettings,
+    jsx: false,
+    compact: settings["graph.label_value_formatting"] === "compact",
+  });


Since we moved scaling to be a simple dataset transformation, we can disregard any scaling property thereafter.

Eventually it would be nice to have scaling only be relevant in the dataset transformation to make it so that value formatters do not even need to consider it.

i.e. Handle the scaling in one place, dataset transformations, rather than having to account for it in all formatting contexts. This way ECharts can know the exact dataset values to determine ranges instead of the unscaled values. Plus we would not have to maintain scaling in value formatting anymore.

JesseSDevaney · 2024-06-05T17:59:01Z

...visualizations/components/ChartTooltip/KeyValuePairChartTooltip/KeyValuePairChartTooltip.tsx

Since value scaling is being applied as a dataset transformation, we want to make sure it is not double applied by the value formatting that occurs in tool-tips.

JesseSDevaney · 2024-06-05T18:00:47Z

frontend/src/metabase/visualizations/echarts/cartesian/model/dataset.ts

+export function scaleDataset(
+  dataset: ChartDataset,
+  seriesModels: SeriesModel[],
+  settings: ComputedVisualizationSettings,
+): ChartDataset {
+  const transformFn = (datum: Datum) => {
+    const transformedRecord = { ...datum };
+    for (const seriesModel of seriesModels) {
+      const scale = getColumnScaling(seriesModel.column, settings);
+
+      const key = seriesModel.dataKey;
+      if (key in datum) {
+        const scaledValue = Number.isFinite(datum[key])
+          ? (datum[key] as number) * scale
+          : null;
+        transformedRecord[key] = scaledValue;
+      }
+    }
+    return transformedRecord;
+  };
+
+  return dataset.map(datum => {
+    return transformFn(datum);
+  });
+}
+


We only need to use the scale factor once for transforming the dataset and then we can disregard the fact that it was ever scaled thereon after. Eventually it would nice to rip out the additional scaling in the value formatter, so that we do not have to maintain that across many different contexts.

JesseSDevaney · 2024-06-05T18:02:04Z

frontend/src/metabase/visualizations/shared/utils/data.ts

Porting the value scaling to RowCharts since they are not implemented as an ECharts chart yet.

github-actions · 2024-06-05T20:48:00Z

@JesseSDevaney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

* return empty string so non-empty labels do not get hidden by empty labels * fix function name * apply scaling in transformed dataset * add missing option to OptionsType * fix double scaling * add column scaling to waterfall transformed dataset * add column value scaling to dataset transform * fix stacked bar segment labels not responding to column scaling * fix tooltip values displayed * since scaling as added to transformedDataset, use transformedDataset for formatters * revert changes to display percentage instead of absolute value * fix stacked data labels * add loki variant for custom y-axis range with column scaling * fix stack segmented label formatters compact calculation * scale original dataset and use that instead of modifying transformedDataset * update loki snapshots * fix scatter plot dataset scaling * fix tooltip values being double scaled * add E2E test to validate behavior * add helper for not duplicating the column scaling * port multiple y-axis metrics with column scaling fixes to RowChart * add waterfall and scatterplot loki variants * fix type errors and failing unit tests * fix failing unit tests * update loki snapshots

* Fix column scaling ("Multiply by a number") bugs (#43668) * return empty string so non-empty labels do not get hidden by empty labels * fix function name * apply scaling in transformed dataset * add missing option to OptionsType * fix double scaling * add column scaling to waterfall transformed dataset * add column value scaling to dataset transform * fix stacked bar segment labels not responding to column scaling * fix tooltip values displayed * since scaling as added to transformedDataset, use transformedDataset for formatters * revert changes to display percentage instead of absolute value * fix stacked data labels * add loki variant for custom y-axis range with column scaling * fix stack segmented label formatters compact calculation * scale original dataset and use that instead of modifying transformedDataset * update loki snapshots * fix scatter plot dataset scaling * fix tooltip values being double scaled * add E2E test to validate behavior * add helper for not duplicating the column scaling * port multiple y-axis metrics with column scaling fixes to RowChart * add waterfall and scatterplot loki variants * fix type errors and failing unit tests * fix failing unit tests * update loki snapshots * update loki snapshots --------- Co-authored-by: Jesse Devaney <22608765+JesseSDevaney@users.noreply.github.com>

JesseSDevaney added backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team labels Jun 5, 2024

JesseSDevaney requested a review from a team June 5, 2024 16:28

JesseSDevaney self-assigned this Jun 5, 2024

JesseSDevaney changed the title ~~Fix column scaling, Multiply by a number, bugs~~ Fix column scaling ("Multiply by a number") bugs Jun 5, 2024

JesseSDevaney changed the title ~~Fix column scaling ("Multiply by a number") bugs~~ Fix column scaling ("Multiply by a number") bugs Jun 5, 2024

JesseSDevaney requested a review from cdeweyx June 5, 2024 17:34

cdeweyx approved these changes Jun 5, 2024

View reviewed changes

JesseSDevaney added 20 commits June 5, 2024 13:52

return empty string so non-empty labels do not get hidden by empty la…

7547960

…bels

fix function name

4c0e992

apply scaling in transformed dataset

27b8b68

add missing option to OptionsType

2f4234e

fix double scaling

a21fe9d

add column scaling to waterfall transformed dataset

3baf109

add column value scaling to dataset transform

0b05127

fix stacked bar segment labels not responding to column scaling

b314191

fix tooltip values displayed

64ed96a

since scaling as added to transformedDataset, use transformedDataset …

693e377

…for formatters

revert changes to display percentage instead of absolute value

573202a

fix stacked data labels

1792925

add loki variant for custom y-axis range with column scaling

efa8727

fix stack segmented label formatters compact calculation

b3e9988

scale original dataset and use that instead of modifying transformedD…

71eddee

…ataset

update loki snapshots

6d368ca

fix scatter plot dataset scaling

90960ca

fix tooltip values being double scaled

cb58da6

add E2E test to validate behavior

f326be4

add helper for not duplicating the column scaling

26f1907

port multiple y-axis metrics with column scaling fixes to RowChart

73a8d70

JesseSDevaney force-pushed the fix-12214/manual-y-axis-range-does-not-take-metric-column-scale-factor-into-account branch from 4ecef1d to 73a8d70 Compare June 5, 2024 17:53

EmmadUsmani approved these changes Jun 5, 2024

View reviewed changes

frontend/src/metabase/visualizations/echarts/cartesian/model/series.ts Show resolved Hide resolved

frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts Show resolved Hide resolved

JesseSDevaney commented Jun 5, 2024

View reviewed changes

JesseSDevaney added 4 commits June 5, 2024 15:01

add waterfall and scatterplot loki variants

bab167b

fix type errors and failing unit tests

8ca0847

fix failing unit tests

c735c86

update loki snapshots

8888d47

JesseSDevaney merged commit 6ac4fe9 into master Jun 5, 2024
111 checks passed

JesseSDevaney deleted the fix-12214/manual-y-axis-range-does-not-take-metric-column-scale-factor-into-account branch June 5, 2024 20:47

github-automation-metabase mentioned this pull request Jun 5, 2024

🤖 backported "Fix column scaling ("Multiply by a number") bugs" #43690

Merged

This was referenced Jun 6, 2024

Row Chart x-axis formatting applies column metric scaling twice #43779

Closed

Fix RowChart X-Axis ticks formatter #43783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix column scaling ("Multiply by a number") bugs #43668

Fix column scaling ("Multiply by a number") bugs #43668

JesseSDevaney commented Jun 5, 2024

github-actions bot commented Jun 5, 2024 •

edited

replay-io bot commented Jun 5, 2024 •

edited

EmmadUsmani left a comment

JesseSDevaney Jun 5, 2024

JesseSDevaney Jun 5, 2024

JesseSDevaney Jun 5, 2024

JesseSDevaney Jun 5, 2024

github-actions bot commented Jun 5, 2024

Fix column scaling ("Multiply by a number") bugs #43668

Fix column scaling ("Multiply by a number") bugs #43668

Conversation

JesseSDevaney commented Jun 5, 2024

Issues Fixed

Manual Y axis min/max don't take the "Multiply by" factor into account (#12214)

Using scaling on a question with multiple y-axis columns breaks tool-tips and y-axis label formatting (#43536)

github-actions bot commented Jun 5, 2024 • edited

replay-io bot commented Jun 5, 2024 • edited

EmmadUsmani left a comment

Choose a reason for hiding this comment

JesseSDevaney Jun 5, 2024

Choose a reason for hiding this comment

JesseSDevaney Jun 5, 2024

Choose a reason for hiding this comment

JesseSDevaney Jun 5, 2024

Choose a reason for hiding this comment

JesseSDevaney Jun 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024 •

edited

replay-io bot commented Jun 5, 2024 •

edited