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:	`2ebd1b1`
Status:	✅ Deploy successful!
Preview URL:	https://f81c430e.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:

src/stack.ts

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

we'd introduce this bug

can't find this link

Fixed link: https://github.com/vega/vega-lite/blob/231ed17c2b37811701a89029ab1524750abdd80b/examples/compiled/arc_quantitative_stacked_theta.png

I found it interesting to see how many of the visual tests break if I removed the check that only applies this change to polar encodings.

#9557

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 · 2025-03-06T19:13:07Z

Shrinking the example down to see if how the issue looks in both coordinate systems on current prod editor (not this branch)

cartesian version of the “bug” (having a common x1) here (pic 1)
polar version of the bug (having a common theta1) here (pic 2)

Cartesian	Polar

Note that in the cartesian case, the stack transform did not get created at all when compiled to Vega (this is because Rect is not supported as a stackable mark?). Whereas in the polar case, the stack transform gets created, but has a groupBy assigned to value just like in the arc case.

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

src/stack.ts Show resolved Hide resolved

kanitw reviewed Feb 18, 2025

View reviewed changes

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

hydrosquall requested a review from kanitw February 23, 2025 17:58

hydrosquall added 2 commits March 2, 2025 20:12

hydrosquall and others added 4 commits March 2, 2025 20:13

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

Verified

This commit was signed with the committer’s verified signature.

hydrosquall Cameron Yick

SSH Key Fingerprint: mLG/6tm4Izt9wtWXhHZtNVixRw+36YCvtNSsHfQc5BQ
Verified
Learn about vigilant mode

ec8762a

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

Verified

This commit was signed with the committer’s verified signature.

hydrosquall Cameron Yick

SSH Key Fingerprint: mLG/6tm4Izt9wtWXhHZtNVixRw+36YCvtNSsHfQc5BQ
Verified
Learn about vigilant mode

9db5065

fix: add test case for quantitative stacked scale theta graphs

Verified

This commit was signed with the committer’s verified signature.

hydrosquall Cameron Yick

SSH Key Fingerprint: mLG/6tm4Izt9wtWXhHZtNVixRw+36YCvtNSsHfQc5BQ
Verified
Learn about vigilant mode

d60e7f9

chore: update examples [CI]

Verified

This commit was signed with the committer’s verified signature.

hydrosquall Cameron Yick

SSH Key Fingerprint: mLG/6tm4Izt9wtWXhHZtNVixRw+36YCvtNSsHfQc5BQ
Verified
Learn about vigilant mode

Loading
Loading status checks…

2ebd1b1

hydrosquall force-pushed the cameron.yick/fix-compile-pie-stack branch from ca62a23 to 2ebd1b1 Compare March 3, 2025 01:13

style: auto-formatting [CI]

Loading
Loading status checks…

158366e

hydrosquall mentioned this pull request Mar 8, 2025

fix(compile): Check if groupBy check can be generalized to non-polar coordinates #9557

Draft

5 tasks

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

hydrosquall Feb 19, 2025

kanitw Mar 6, 2025

hydrosquall Mar 6, 2025

hydrosquall Mar 8, 2025

kanitw left a comment

hydrosquall commented Mar 6, 2025 •

edited

Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

hydrosquall commented Mar 6, 2025 • edited Loading

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 commented Mar 6, 2025 •

edited

Loading