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 10 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 @@ -8143,6 +8143,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:__ `\"independent\"`",
"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
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": "independent"
},
{
"type": "impute",
Expand Down
Binary file modified examples/compiled/area_density_facet.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_facet.svg
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_facet.vg.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"groupby": ["Species"],
"extent": [2500, 6500],
"as": ["value", "density"],
"resolve": "shared"
"resolve": "independent"
},
{
"type": "impute",
Expand Down
Binary file modified examples/compiled/area_density_stacked.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_stacked.svg
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_stacked.vg.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"groupby": ["Species"],
"extent": [2500, 6500],
"as": ["value", "density"],
"resolve": "shared"
"resolve": "independent"
},
{
"type": "impute",
Expand Down
2 changes: 1 addition & 1 deletion examples/compiled/area_density_stacked_fold.vg.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"counts": true,
"steps": 200,
"as": ["value", "density"],
"resolve": "shared"
"resolve": "independent"
},
{
"type": "impute",
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 ?? 'independent';
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.
joelostblom marked this conversation as resolved.
Show resolved Hide resolved
*
* __Default value:__ `"independent"`
*/
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 @@ -27,7 +27,7 @@ describe('compile/data/fold', () => {
extent: [0, 10],
minsteps: 25,
maxsteps: 200,
resolve: 'shared',
resolve: 'independent',
as: ['x', 'y']
});
});
Expand All @@ -41,6 +41,7 @@ describe('compile/data/fold', () => {
expect(density.assemble()).toEqual({
type: 'kde',
field: 'v',
resolve: 'independent',
as: ['value', 'density']
});
});
Expand All @@ -54,14 +55,16 @@ describe('compile/data/fold', () => {
expect(density.assemble()).toEqual({
type: 'kde',
field: 'v',
resolve: 'independent',
as: ['A', 'density']
});
});

it('should add resolve shared if we group', () => {
it('should only add resolve "shared" if we set it explicitly', () => {
Copy link
Member

Choose a reason for hiding this comment

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

To test this, we would need a case that doesn't set resolve when it's not set in the transform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't resolve always be in the transform sine the parameter has a default value? I think this test case should just be switched to "independent" instead of "shared" to reflect the updates in e3f4505. I did that in my latest commit.

const transform: Transform = {
density: 'v',
groupby: ['a']
groupby: ['a'],
resolve: 'shared'
};
const density = new DensityTransformNode(null, transform);
expect(density.assemble()).toEqual({
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":"independent"}');
});
});

Expand Down