-
Notifications
You must be signed in to change notification settings - Fork 43
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
Changes from 7 commits
d460213
c1a99d8
435c733
e811e39
5c5488a
e9eac3d
8d3bed4
5600bcb
c9a4dd5
6e59c50
3213642
efa4898
1452dd6
fc97be4
733680b
2293acd
510255b
014bd8c
f0a9665
4e5ba6f
be8149c
34e9917
f0bfe63
682921f
c4e21f3
234e71c
ece9ee7
3f2cfb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,12 @@ export interface QueryConfig { | |
// Effectiveness Preference | ||
maxGoodCardinalityForColor?: number; // FIXME: revise | ||
maxGoodCardinalityForFacet?: number; // FIXME: revise | ||
|
||
// POST ENUMERATION MODIFICATION | ||
smallBandSizeForHighCardinality?: {maxCardinality: number, bandSize: number}; | ||
smallBandSizeForFacet?: {maxCardinality: number, bandSize: number}; | ||
nominalScaleForHighCardinality?: {maxCardinality: number}; | ||
|
||
} | ||
|
||
export const DEFAULT_QUERY_CONFIG: QueryConfig = { | ||
|
@@ -160,4 +166,9 @@ export const DEFAULT_QUERY_CONFIG: QueryConfig = { | |
|
||
maxGoodCardinalityForFacet: 5, // FIXME: revise | ||
maxGoodCardinalityForColor: 7, // FIXME: revise | ||
|
||
// POST ENUMERATION MODIFICATION | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}, | ||
}; |
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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> = {}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I already mentioned that you should break sub-logic in Break logic in this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Use |
||
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 || {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why do you need to add
You either do
or simply
which I prefer because it's shorter. The one you're writing doesn't make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good call about that. However, you still don't need 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. (Break this into and method and remove your extra |
||
|
||
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; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this above
and change Effectiveness Preference to ALL CAPS.