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: add explicit option to control how densities are resolved, change how densities are resolved by default #9172

Merged
merged 19 commits into from
Mar 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions build/vega-lite-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -8145,6 +8145,14 @@
"description": "The minimum number of samples to take along the extent domain for plotting the density.\n\n__Default value:__ `25`",
"type": "number"
},
"resolve": {
"description": "Indicates how parameters for multiple densities should be resolved. If `\"independent\"`, each density may have its own domain extent and dynamic number of curve sample steps. If `\"shared\"`, the KDE transform will ensure that all densities are defined over a shared domain and curve steps, enabling stacking.\n\n__Default value:__ `\"shared\"`",
"enum": [
"independent",
"shared"
],
"type": "string"
},
"steps": {
"description": "The exact number of samples to take along the extent domain for plotting the density. If specified, overrides both minsteps and maxsteps to set an exact number of uniform samples. Potentially useful in conjunction with a fixed extent to ensure consistent sample points for stacked densities.",
"type": "number"
Expand Down
Binary file modified examples/compiled/area_density.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion examples/compiled/area_density.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 2 additions & 1 deletion examples/compiled/area_density.vg.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"type": "kde",
"field": "IMDB Rating",
"bandwidth": 0.3,
"as": ["value", "density"]
"as": ["value", "density"],
"resolve": "shared"
Copy link
Member

Choose a reason for hiding this comment

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

This used to be independent. https://vega.github.io/vega/docs/transforms/kde/

Why should this be shared for non-stacked transforms? Maybe a reasonable default is to have it only shared if there is stacking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly from our discussion, we said it would be difficult to set the default value of the transform based on whether the encoding channel is stacked or not. I believe the main reason was that it would be hard to detect which encoding channel the density values are plotted on.

Leaving "shared" as the default would work in most situations, including most of the common ones, and if there are situations where the "independent" is needed, it would be easy to switch. To avoid that densities stack by default, we said we would turn off stacking as part of a mark_density, but leave the default stacking behavior for the area mark as it is now to be consistent with other marks.

},
{
"type": "impute",
Expand Down
2 changes: 1 addition & 1 deletion site/docs/transform/density.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ The density transform performs one-dimensional [kernel density estimation](https

## Density Transform Definition

{% include table.html props="density,groupby,cumulative,counts,bandwidth,extent,minsteps,maxsteps,steps,as" source="DensityTransform" %}
{% include table.html props="density,groupby,cumulative,counts,bandwidth,extent,minsteps,maxsteps,resolve,steps,as" source="DensityTransform" %}

## Usage

Expand Down
6 changes: 3 additions & 3 deletions src/compile/data/density.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export class DensityTransformNode extends DataFlowNode {
this.transform = duplicate(transform); // duplicate to prevent side effects
const specifiedAs = this.transform.as ?? [undefined, undefined];
this.transform.as = [specifiedAs[0] ?? 'value', specifiedAs[1] ?? 'density'];
const resolve = this.transform.resolve ?? 'shared';
this.transform.resolve = resolve;
}

public dependentFields() {
Expand All @@ -40,9 +42,7 @@ export class DensityTransformNode extends DataFlowNode {
field: density,
...rest
};
if (this.transform.groupby) {
result.resolve = 'shared';
}
result.resolve = this.transform.resolve;
return result;
}
}
8 changes: 8 additions & 0 deletions src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,14 @@ export interface DensityTransform {
* __Default value:__ `["value", "density"]`
*/
as?: [FieldName, FieldName];
/**
* Indicates how parameters for multiple densities should be resolved.
* If `"independent"`, each density may have its own domain extent and dynamic number of curve sample steps.
* If `"shared"`, the KDE transform will ensure that all densities are defined over a shared domain and curve steps, enabling stacking.
*
* __Default value:__ `"shared"`
*/
resolve?: 'independent' | 'shared';
}

export function isDensity(t: Transform): t is DensityTransform {
Expand Down
11 changes: 7 additions & 4 deletions test/compile/data/density.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ describe('compile/data/fold', () => {
expect(density.assemble()).toEqual({
type: 'kde',
field: 'v',
resolve: 'shared',
as: ['value', 'density']
});
});
Expand All @@ -54,21 +55,23 @@ describe('compile/data/fold', () => {
expect(density.assemble()).toEqual({
type: 'kde',
field: 'v',
resolve: 'shared',
as: ['A', 'density']
});
});

it('should add resolve shared if we group', () => {
it('should add resolve "independent" if we set it explicitly', () => {
const transform: Transform = {
density: 'v',
groupby: ['a']
groupby: ['a'],
resolve: 'independent'
};
const density = new DensityTransformNode(null, transform);
expect(density.assemble()).toEqual({
type: 'kde',
groupby: ['a'],
field: 'v',
resolve: 'shared',
resolve: 'independent',
as: ['value', 'density']
});
});
Expand Down Expand Up @@ -119,7 +122,7 @@ describe('compile/data/fold', () => {
as: ['A', 'B']
};
const density = new DensityTransformNode(null, transform);
expect(density.hash()).toBe('DensityTransform {"as":["A","B"],"density":"v"}');
expect(density.hash()).toBe('DensityTransform {"as":["A","B"],"density":"v","resolve":"shared"}');
});
});

Expand Down