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.

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

feat: Area with plain quantitative fields on both axes get stacked by default #9018

Merged
merged 5 commits into from
Jul 20, 2023

Conversation

yhoonkim
Copy link
Contributor

@yhoonkim yhoonkim commented Jul 19, 2023

Address this issue: #9001

@yhoonkim yhoonkim changed the title Yh/area default stack feat: Area with plain quantitative fields on both axes get stacked by default Jul 19, 2023
@yhoonkim yhoonkim force-pushed the yh/area-default-stack branch from a26d87d to 98e0ebe Compare July 19, 2023 20:25
@@ -14,7 +14,8 @@
},
"y": {
"field": "Cumulative Count",
"type": "quantitative"
"type": "quantitative",
"aggregate": "max"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this PR fixes this example which uses window transform and plot the chart but without aggregation. The backed data is not aggregated so internally it draws many more unnecessary data points.

Those unnecessary data points break the area chart with the new default stack in area.

Copy link
Member

Choose a reason for hiding this comment

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

(nit) A more efficient fix is probably aggregate the count before calling window, but feel free to file a follow up ticket if u don't wanna fix now.

@yhoonkim yhoonkim requested review from kanitw and domoritz July 19, 2023 20:32
@yhoonkim yhoonkim marked this pull request as ready for review July 19, 2023 20:33
@@ -130,7 +132,7 @@ export class PathOverlayNormalizer implements NonFacetUnitNormalizer<UnitSpecWit
// FIXME: determine rules for applying selections.

// Need to copy stack config to overlayed layer
const stackProps = stack(markDef, encoding);
const stackProps = stack(initMarkdef(markDef, encoding, config), encoding);
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a tech debt, since normalizer shouldn't call methods from an init phase.

So I'd at least add

// FIXME: normalizer shouldn't call methods from an init phase. 

@@ -14,7 +14,8 @@
},
"y": {
"field": "Cumulative Count",
"type": "quantitative"
"type": "quantitative",
"aggregate": "max"
Copy link
Member

Choose a reason for hiding this comment

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

(nit) A more efficient fix is probably aggregate the count before calling window, but feel free to file a follow up ticket if u don't wanna fix now.

@kanitw kanitw enabled auto-merge (squash) July 19, 2023 23:52
@yhoonkim yhoonkim force-pushed the yh/area-default-stack branch from d073d29 to ec6e2e1 Compare July 20, 2023 04:53
@kanitw kanitw merged commit b79bc9f into main Jul 20, 2023
@kanitw kanitw deleted the yh/area-default-stack branch July 20, 2023 05:03
BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
@joelostblom
Copy link
Contributor

joelostblom commented Nov 11, 2023

@yhoonkim @kanitw I have noticed issues with densities getting stacked by default when using the area mark and I believe it comes from this PR, see #9170 for an example. This is undesirable because it makes it hard to compare the two distributions with each other. Is there any way to make this PR compatible with the fact that we don't want to stack densities by default (while one could use a line for the density transform, it is more common to use an area and it is unexpected that they differ as well)? Stacking the density areas used to require the stack to be explicitly set like in this example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants