fix: Arc mark does not stack for `quantitative` definitions when different fields are used for `theta` and `radius` [draft] #9512

hydrosquall · 2025-01-20T16:06:20Z

Motivation

When using different marks for theta vs radius using an arc mark with stacks, change graph rendering
Fixes Radial chart radius field unexpectedly changes arc stack behavior #7957

Changes

Do not include groupBy field when the dimension field is using a quantitative scale. This ensures that stacking is applied correctly.
I've made this change only impact polar coordinate channels to keep the impact radius minimal since this customization may be specific to the arc mark.
Try the VL Spec from the original bug report
- preview
- You can also check with the colorBy case
- For comparison this shape is cool, and shows we can still get something useful when the stack is applied on radius rather than theta

Testing

API design: does this approach make sense for how stacks should apply for arc marks?
See unit test suite : Let me know if additional cases should be added to either the image snapshots or the unit test suite.
Compare with similar cartesian plot

cloudflare-workers-and-pages · 2025-01-20T16:15:37Z

Deploying vega-lite with Cloudflare Pages

Latest commit:	`ca62a23`
Status:	✅ Deploy successful!
Preview URL:	https://d4c82713.vega-lite.pages.dev
Branch Preview URL:	https://cameron-yick-fix-compile-pie.vega-lite.pages.dev

View logs

hydrosquall · 2025-01-20T16:19:52Z

src/stack.ts

      // avoid grouping by the stacked field
+      // TKTK: find out why


If this works here, we may need a similar change for the code block under dimensionOffsetChannel .

hydrosquall · 2025-01-20T16:28:52Z

src/stack.ts

@@ -182,7 +182,7 @@ export function stack(m: Mark | MarkDef, encoding: Encoding<string>): StackPrope
    // TBD: we should only enter this branch if there's also a "color" and maybe a group channel?
    const isPolar = isPolarPositionChannel(fieldChannel) || isPolarPositionChannel(dimensionChannel);

-    if (!hasSameDimensionAndStackedField && (isPolar ? dimensionChannel === fieldChannel || encoding['color'] : true)) {
+    if (!hasSameDimensionAndStackedField && (isPolar ? dimensionChannel === fieldChannel : true)) {


Before I removed the color encoding check, the graph rendered like this:

kanitw · 2025-02-18T16:28:38Z

src/stack.ts

@@ -176,8 +176,13 @@ export function stack(m: Mark | MarkDef, encoding: Encoding<string>): StackPrope
  if (encoding[dimensionChannel]) {
    const dimensionDef = encoding[dimensionChannel];
    const dimensionField = isFieldDef(dimensionDef) ? vgField(dimensionDef, {}) : undefined;
+    const hasSameDimensionAndStackedField = dimensionField && dimensionField === stackedField;


just having the same input field doesn't mean that it's the same transformed field? I think we need to check if they have the same aggregate/bin/timeUnit?

Ah interesting. Do you know if we have helper utilities for checking if two encodings are "equivalent"? (I.e. matching in field name and any relevant inline transformations like bin/timeUnit/bin?

I looked in channelDef just now and didn't see anything that matches. I can add one if it seems like it would be appropriate here.

kanitw · 2025-02-18T16:29:00Z

src/stack.ts

+    const isPolar = isPolarPositionChannel(fieldChannel) || isPolarPositionChannel(dimensionChannel);
+    const shouldAddPolarGroupBy = !isUnbinnedQuantitative(dimensionDef);
+
+    if (isPolar ? shouldAddPolarGroupBy : !hasSameDimensionAndStackedField) {


I wonder if we really need isPolar ? shouldAddPolarGroupBy : clause? If we strictly check the field above, would that be sufficient?

By strictly check, do you mean we'd check if dimensionDef and stackedFieldDef have the same

field (the current check)

bin

aggregator

timeUnit

for both polar and cartesian graphs?

Currently if I just remove the isPolar ? shouldAddPolarGroupBy clause and use the existing check, we'd introduce this bug

Made a PoC commit to see if this is what you had in mind

8b010a6

kanitw

Thanks for the PR. I think the test and regression case look good, but I wonder if the condition added can be more precise/simpler.

…el and stackChannel are same

…hannels are matching

hydrosquall changed the title ~~[draft]~~ [draft] Radial chart does not render as expected when different fields are used for theta and radius Jan 20, 2025

hydrosquall changed the title ~~[draft] Radial chart does not render as expected when different fields are used for theta and radius~~ fix: Radial chart does not render as expected when different fields are used for theta and radius [draft] Jan 20, 2025

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch 2 times, most recently from 0b7380a to 7f6ed6d Compare January 20, 2025 16:15

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch from 7f6ed6d to 3b1d979 Compare January 20, 2025 16:16

hydrosquall changed the title ~~fix: Radial chart does not render as expected when different fields are used for theta and radius [draft]~~ fix: Radial mark does not render as expected when different fields used for theta and radius [draft] Jan 20, 2025

hydrosquall commented Jan 20, 2025

View reviewed changes

hydrosquall mentioned this pull request Jan 20, 2025

Radial chart radius field unexpectedly changes arc stack behavior #7957

Open

5 tasks

hydrosquall self-assigned this Jan 20, 2025

hydrosquall commented Jan 20, 2025

View reviewed changes

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch 5 times, most recently from 89b647c to 369fa1e Compare January 26, 2025 02:31

hydrosquall changed the title ~~fix: Radial mark does not render as expected when different fields used for theta and radius [draft]~~ fix: Radial mark does not stack for quantitative definitions when different fields are used for theta and radius [draft] Jan 26, 2025

hydrosquall changed the title ~~fix: Radial mark does not stack for quantitative definitions when different fields are used for theta and radius [draft]~~ fix: Arc mark does not stack for quantitative definitions when different fields are used for theta and radius [draft] Jan 26, 2025

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch from a21fed9 to 905e53c Compare January 26, 2025 02:55

hydrosquall marked this pull request as ready for review January 26, 2025 03:23

hydrosquall requested a review from a team as a code owner January 26, 2025 03:23

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch from 905e53c to 6d37199 Compare January 30, 2025 05:02

kanitw reviewed Feb 18, 2025

View reviewed changes

hydrosquall added 5 commits February 18, 2025 23:02

fix(stack): do not add field to a stack's groupBy unless groupByChann…

f56a29e

…el and stackChannel are same

test: check ensure groupBy is not added for stack when radial chart c…

a9ac9ff

…hannels are matching

test(stack): limit radius of change to polar coordinate graphs

8424de9

fix: settle stacked arc marks with a check for field type

1e36ff9

fix: add test case for quantitative stacked scale theta graphs

c81bef8

chore: update examples [CI]

ca62a23

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch from 6d37199 to ca62a23 Compare February 19, 2025 04:02

hydrosquall mentioned this pull request Feb 19, 2025

test: show effect of checking for same dimension and stacked field in polar and cartesian fields #9537

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Arc mark does not stack for `quantitative` definitions when different fields are used for `theta` and `radius` [draft] #9512

fix: Arc mark does not stack for `quantitative` definitions when different fields are used for `theta` and `radius` [draft] #9512

hydrosquall commented Jan 20, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 20, 2025 •

edited

Loading

hydrosquall Jan 20, 2025

hydrosquall Jan 20, 2025

kanitw Feb 18, 2025

hydrosquall Feb 19, 2025

kanitw Feb 18, 2025

hydrosquall Feb 19, 2025

hydrosquall Feb 19, 2025

kanitw left a comment

fix: Arc mark does not stack for quantitative definitions when different fields are used for theta and radius [draft] #9512

Are you sure you want to change the base?

fix: Arc mark does not stack for quantitative definitions when different fields are used for theta and radius [draft] #9512

Conversation

hydrosquall commented Jan 20, 2025 • edited Loading

Motivation

Changes

Testing

cloudflare-workers-and-pages bot commented Jan 20, 2025 • edited Loading

Deploying vega-lite with Cloudflare Pages

hydrosquall Jan 20, 2025

Choose a reason for hiding this comment

hydrosquall Jan 20, 2025

Choose a reason for hiding this comment

kanitw Feb 18, 2025

Choose a reason for hiding this comment

hydrosquall Feb 19, 2025

Choose a reason for hiding this comment

kanitw Feb 18, 2025

Choose a reason for hiding this comment

hydrosquall Feb 19, 2025

Choose a reason for hiding this comment

hydrosquall Feb 19, 2025

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

fix: Arc mark does not stack for `quantitative` definitions when different fields are used for `theta` and `radius` [draft] #9512

fix: Arc mark does not stack for `quantitative` definitions when different fields are used for `theta` and `radius` [draft] #9512

hydrosquall commented Jan 20, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 20, 2025 •

edited

Loading