Skip to content

Commit

Permalink
fix: use resolve "shared" instead of steps to fix artifacts in groupe…
Browse files Browse the repository at this point in the history
…d density transform (#9106)

* Use resolve shared instead of steps

* Update density tests

* chore: update examples [CI]

---------

Co-authored-by: GitHub Actions Bot <[email protected]>
  • Loading branch information
jonmmease and GitHub Actions Bot authored Sep 20, 2023
1 parent d45fa23 commit 06e63e6
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 10 deletions.
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"],
"steps": 200
"resolve": "shared"
},
{
"type": "impute",
Expand Down
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"],
"steps": 200
"resolve": "shared"
},
{
"type": "impute",
Expand Down
3 changes: 2 additions & 1 deletion examples/compiled/area_density_stacked_fold.vg.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"extent": [10, 500],
"counts": true,
"steps": 200,
"as": ["value", "density"]
"as": ["value", "density"],
"resolve": "shared"
},
{
"type": "impute",
Expand Down
8 changes: 3 additions & 5 deletions src/compile/data/density.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,6 @@ 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'];

// set steps when we are grouping so that we get consitent sampling points for imputing and grouping
if (transform.groupby && transform.minsteps == null && transform.maxsteps == null && transform.steps == null) {
this.transform.steps = 200;
}
}

public dependentFields() {
Expand All @@ -45,6 +40,9 @@ export class DensityTransformNode extends DataFlowNode {
field: density,
...rest
};
if (this.transform.groupby) {
result.resolve = 'shared';
}
return result;
}
}
5 changes: 3 additions & 2 deletions test/compile/data/density.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ describe('compile/data/fold', () => {
extent: [0, 10],
minsteps: 25,
maxsteps: 200,
resolve: 'shared',
as: ['x', 'y']
});
});
Expand Down Expand Up @@ -57,7 +58,7 @@ describe('compile/data/fold', () => {
});
});

it('should add steps if we group', () => {
it('should add resolve shared if we group', () => {
const transform: Transform = {
density: 'v',
groupby: ['a']
Expand All @@ -67,7 +68,7 @@ describe('compile/data/fold', () => {
type: 'kde',
groupby: ['a'],
field: 'v',
steps: 200,
resolve: 'shared',
as: ['value', 'density']
});
});
Expand Down

0 comments on commit 06e63e6

Please sign in to comment.