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

Add post enumeration modification to generate.ts #187

Merged
merged 28 commits into from
Aug 8, 2016
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d460213
add post enumeration modification to generate.ts
espressoroaster Aug 7, 2016
c1a99d8
move post enumeration modification
espressoroaster Aug 7, 2016
435c733
ENCODING_QUERY_INDEX => encQIndex
espressoroaster Aug 7, 2016
e811e39
put in one reduce and add encQPositionIndex dict
espressoroaster Aug 8, 2016
5c5488a
create stylize.ts
espressoroaster Aug 8, 2016
e9eac3d
Directly access EncQ through yEncQ
espressoroaster Aug 8, 2016
8d3bed4
remove unused import and variable declaration
espressoroaster Aug 8, 2016
5600bcb
update config description
espressoroaster Aug 8, 2016
c9a4dd5
Update name of Stylize config
espressoroaster Aug 8, 2016
6e59c50
Add Stylize.test.ts
espressoroaster Aug 8, 2016
3213642
add generate test for nominalScaleForHighCardinality and other config
espressoroaster Aug 8, 2016
efa4898
add stylize test and generate test case for smallBandSize due to facet
espressoroaster Aug 8, 2016
1452dd6
nominalScaleForHighCardinality --> nominalColorScaleForHighCardinality
espressoroaster Aug 8, 2016
fc97be4
add generate test with nominalColorScaleForHighCardinality config turned
espressoroaster Aug 8, 2016
733680b
move to test to 1D
espressoroaster Aug 8, 2016
2293acd
add palettte string to nominalColorScaleForHighCardinality config
espressoroaster Aug 8, 2016
510255b
Q -> O,
espressoroaster Aug 8, 2016
014bd8c
allow bandSize to be set when scale is enumspec
espressoroaster Aug 8, 2016
f0a9665
add and yEncQ's scaleType is ordinal for bandSize config
espressoroaster Aug 8, 2016
4e5ba6f
nest stylizer tests under describe(stylizer)
espressoroaster Aug 8, 2016
be8149c
allow bandSize to be set if encQ's scale type is enum spec
espressoroaster Aug 8, 2016
34e9917
stylize should not assign a bandSize of 12 if bandSize is already set
espressoroaster Aug 8, 2016
f0bfe63
stylizer should not assign a range if range is already set
espressoroaster Aug 8, 2016
682921f
remove extra space
espressoroaster Aug 8, 2016
c4e21f3
Add missing //
kanitw Aug 8, 2016
234e71c
use config values for maxCardinality and palette range
espressoroaster Aug 8, 2016
ece9ee7
stylize should not assign a bandSize if cardinality of Y is under 10
espressoroaster Aug 8, 2016
3f2cfb1
do not assign a range of category20 if cardinality of color is under 10
espressoroaster Aug 8, 2016
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
11 changes: 11 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ export interface QueryConfig {
// Effectiveness Preference
maxGoodCardinalityForColor?: number; // FIXME: revise
maxGoodCardinalityForFacet?: number; // FIXME: revise

// POST ENUMERATION MODIFICATION
Copy link
Member

Choose a reason for hiding this comment

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

We call it stylize - this is inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

Move this above

// Effectiveness Preference

and change Effectiveness Preference to ALL CAPS.

smallBandSizeForHighCardinality?: {maxCardinality: number, bandSize: number};
smallBandSizeForFacet?: {maxCardinality: number, bandSize: number};
nominalScaleForHighCardinality?: {maxCardinality: number};

}

export const DEFAULT_QUERY_CONFIG: QueryConfig = {
Expand Down Expand Up @@ -160,4 +166,9 @@ export const DEFAULT_QUERY_CONFIG: QueryConfig = {

maxGoodCardinalityForFacet: 5, // FIXME: revise
maxGoodCardinalityForColor: 7, // FIXME: revise

// POST ENUMERATION MODIFICATION
Copy link
Member

Choose a reason for hiding this comment

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

stylize

smallBandSizeForHighCardinality: {maxCardinality: 10, bandSize: 12},
smallBandSizeForFacet: {maxCardinality: 10, bandSize: 12},
nominalScaleForHighCardinality: {maxCardinality: 10},
};
3 changes: 2 additions & 1 deletion src/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {QueryConfig, DEFAULT_QUERY_CONFIG} from './config';
import {SpecQueryModel} from './model';
import {SpecQuery} from './query/spec';
import {Schema} from './schema';
import {stylize} from './stylize';

export function generate(specQ: SpecQuery, schema: Schema, opt: QueryConfig = DEFAULT_QUERY_CONFIG) {
// 1. Build a SpecQueryModel, which also contains enumSpecIndex
Expand All @@ -22,5 +23,5 @@ export function generate(specQ: SpecQuery, schema: Schema, opt: QueryConfig = DE
}
});

return answerSet;
return stylize(answerSet, schema, opt);
}
74 changes: 74 additions & 0 deletions src/stylize.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import {Channel} from 'vega-lite/src/channel';
import {Type} from 'vega-lite/src/type';

import {QueryConfig} from './config';
import {SpecQueryModel} from './model';
// import {Property} from './property';
Copy link
Member

Choose a reason for hiding this comment

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

Why leaving // ?

import {EncodingQuery, ScaleQuery} from './query/encoding';
import {Schema} from './schema';
import {Dict} from './util';

export function stylize(answerSet: SpecQueryModel[], schema: Schema, opt: QueryConfig): SpecQueryModel[] {

answerSet = answerSet.map(function(specM) {
let encQIndex: Dict<EncodingQuery> = {};

Copy link
Member

Choose a reason for hiding this comment

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

not sure why empty line here.

if (opt.smallBandSizeForHighCardinality || opt.smallBandSizeForFacet) {

[Channel.ROW, Channel.Y, Channel.COLUMN, Channel.X].forEach((channel) => {
Copy link
Member

Choose a reason for hiding this comment

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

I already mentioned that you should break sub-logic in stylize into multiple methods.

Break logic in this if into a method.

Copy link
Member

Choose a reason for hiding this comment

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

getEncodingQueryByChannel takes O(n) where n = # of channels and you call this 4 times.

Use specM.encodings() and reduce them to create index.

encQIndex[channel] = specM.getEncodingQueryByChannel(channel);
});

const yEncQ = encQIndex[Channel.Y];
if (yEncQ !== undefined) {
if (encQIndex[Channel.ROW] ||
schema.cardinality(yEncQ) > 10) {

if (yEncQ.scale === undefined) {
yEncQ.scale = yEncQ.scale || {};
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why do you need to add

 if (yEncQ.scale === undefined) 

You either do

if (yEncQ.scale === undefined) {
  yEncQ.scale = {};
}

or simply

yEncQ.scale = yEncQ.scale || {};

which I prefer because it's shorter.

The one you're writing doesn't make sense.

Copy link
Member Author

@espressoroaster espressoroaster Aug 8, 2016

Choose a reason for hiding this comment

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

I was thinking of the case where yEncQ.scale was explictly set to false. If we do
yEncQ.scale = yEncQ.scale || {} , we would assign {} to yEncQ.scale in addition to assigning a bandSize although the scale was initially set to false.

Copy link
Member

Choose a reason for hiding this comment

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

Oh good call about that. However, you still don't need yEncQ.scale = yEncQ.scale || {}; when scale is already undefined.

Because of that I couldn't guess that you think about this from reading (distracted). Plus, there are no comment explaining and no test case.

Can you add comment saying why we have to explicitly check for undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Ok now that you think about false/null case. We should also consider when scale / scale.bandWidth are enumSpec / have existing values as well.

}

if (yEncQ.scale) {
(yEncQ.scale as ScaleQuery).bandSize = 12;
}
}
}

const xEncQ = encQIndex[Channel.X];
if (xEncQ !== undefined) {
if (encQIndex[Channel.COLUMN] ||
schema.cardinality(xEncQ) > 10) {

if (xEncQ.scale === undefined) {
xEncQ.scale = xEncQ.scale || {};
}

if (xEncQ.scale) {
(xEncQ.scale as ScaleQuery).bandSize = 12;
}
}
}
}

if (opt.nominalScaleForHighCardinality) {

encQIndex[Channel.COLOR] = specM.getEncodingQueryByChannel(Channel.COLOR);
Copy link
Member

@kanitw kanitw Aug 8, 2016

Choose a reason for hiding this comment

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

Same here. (Break this into and method and remove your extra if clauses.


const colorEncQ = encQIndex[Channel.COLOR];
if ((colorEncQ !== undefined) && (colorEncQ.type === Type.NOMINAL) &&
(schema.cardinality(colorEncQ) > 10)) {

if (colorEncQ.scale === undefined) {
colorEncQ.scale = colorEncQ.scale || {};
}

if (colorEncQ.scale) {
(colorEncQ.scale as ScaleQuery).range = 'category20';
}
}
}
return specM;
});

return answerSet;
}
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"src/nest.ts",
"src/property.ts",
"src/schema.ts",
"src/stylize.ts",
"src/util.ts",
"test/constraint/encoding.test.ts",
"test/constraint/spec.test.ts",
Expand Down