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

Support field in condition #1980

Merged
merged 16 commits into from
Jun 11, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 2 additions & 3 deletions src/compile/mark/mark.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ import {UnitModel} from '../unit';

import {isArray} from 'vega-util';
import {X, Y} from '../../channel';
import {getFieldDef} from '../../encoding';
import {field} from '../../fielddef';
import {field, getFieldDef} from '../../fielddef';
import {isSelectionDomain} from '../../scale';

const markCompiler: {[type: string]: MarkCompiler} = {
Expand Down Expand Up @@ -131,7 +130,7 @@ function detailFields(model: UnitModel): string[] {
});
}
} else {
const fieldDef = getFieldDef(encoding, channel);
const fieldDef = getFieldDef<string>(encoding[channel]);
if (fieldDef && !fieldDef.aggregate) {
details.push(field(fieldDef, {binSuffix: 'start'}));
}
Expand Down
6 changes: 4 additions & 2 deletions src/compile/mark/mixins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {UnitModel} from '../unit';
import * as ref from './valueref';

import {NONSPATIAL_SCALE_CHANNELS} from '../../channel';
import {Condition, FieldDef, isFieldDef, isValueDef} from '../../fielddef';
import {Condition, FieldDef, getFieldDef, isValueDef} from '../../fielddef';
import {predicate} from '../selection/selection';

export function color(model: UnitModel) {
Expand Down Expand Up @@ -63,6 +63,7 @@ export function nonPosition(channel: typeof NONSPATIAL_SCALE_CHANNELS[0], model:
* Return a mixin that include a Vega production rule for a Vega-Lite conditional channel definition.
* or a simple mixin if channel def has no condition.
*/
// FIXME support ConditionFieldDef
function wrapCondition(model: UnitModel, condition: Condition<any>, vgChannel: string, valueRef: VgValueRef): VgEncodeEntry {
if (condition) {
const {selection, value} = condition;
Expand All @@ -77,6 +78,7 @@ function wrapCondition(model: UnitModel, condition: Condition<any>, vgChannel: s
}
}

// FIXME support ConditionFieldDef
export function text(model: UnitModel, vgChannel: 'text' | 'tooltip' = 'text') {
const channelDef = model.encoding[vgChannel];
const valueRef = (vgChannel === 'tooltip' && !channelDef) ? undefined : ref.text(channelDef, model.config);
Expand All @@ -96,7 +98,7 @@ export function bandPosition(fieldDef: FieldDef<string>, channel: 'x'|'y', model
[channel+'c']: ref.fieldRef(fieldDef, scaleName, {}, {band: 0.5})
};

if (isFieldDef(model.encoding.size)) {
if (getFieldDef(model.encoding.size)) {
log.warn(log.message.cannotUseSizeFieldWithBandSize(channel));
// TODO: apply size to band and set scale range to some values between 0-1.
// return {
Expand Down
1 change: 1 addition & 0 deletions src/compile/mark/valueref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export function midPoint(channel: Channel, channelDef: ChannelDef<string>, scale

if (channelDef) {
/* istanbul ignore else */

if (isFieldDef(channelDef)) {
if (isBinScale(scale.type)) {
// Use middle only for x an y to place marks in the center between start and end of the bin range.
Expand Down
13 changes: 9 additions & 4 deletions src/compile/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {Channel, COLUMN, isChannel, NonspatialScaleChannel, ScaleChannel, X} fro
import {CellConfig, Config} from '../config';
import {Data, DataSourceType, MAIN, RAW} from '../data';
import {forEach, reduce} from '../encoding';
import {ChannelDef, field, FieldDef, FieldRefOption, isFieldDef, isRepeatRef} from '../fielddef';
import {ChannelDef, field, FieldDef, FieldRefOption, getFieldDef, isFieldDef, isRepeatRef} from '../fielddef';
import {Legend} from '../legend';
import {hasDiscreteDomain, Scale} from '../scale';
import {SortField, SortOrder} from '../sort';
Expand Down Expand Up @@ -400,14 +400,19 @@ export abstract class ModelWithField extends Model {

public reduceFieldDef<T, U>(f: (acc: U, fd: FieldDef<string>, c: Channel) => U, init: T, t?: any) {
return reduce(this.getMapping(), (acc:U , cd: ChannelDef<string>, c: Channel) => {
return isFieldDef(cd) ? f(acc, cd, c) : acc;
const fieldDef = getFieldDef(cd);
if (fieldDef) {
return f(acc, fieldDef, c);
}
return acc;
}, init, t);
}

public forEachFieldDef(f: (fd: FieldDef<string>, c: Channel) => void, t?: any) {
forEach(this.getMapping(), (cd: ChannelDef<string>, c: Channel) => {
if (isFieldDef(cd)) {
f(cd, c);
const fieldDef = getFieldDef(cd);
if (fieldDef) {
f(fieldDef, c);
}
}, t);
}
Expand Down
20 changes: 13 additions & 7 deletions src/compile/unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {Channel, NONSPATIAL_SCALE_CHANNELS, UNIT_CHANNELS, UNIT_SCALE_CHANNELS,
import {CellConfig, Config} from '../config';
import {Encoding, normalizeEncoding} from '../encoding';
import * as vlEncoding from '../encoding'; // TODO: remove
import {field, FieldDef, FieldRefOption, isFieldDef} from '../fielddef';
import {field, FieldDef, FieldRefOption, getFieldDef, isConditionalDef, isFieldDef} from '../fielddef';
import {Legend} from '../legend';
import {FILL_STROKE_CONFIG, isMarkDef, Mark, MarkDef, TEXT as TEXT_MARK} from '../mark';
import {defaultScaleConfig, Domain, hasDiscreteDomain, Scale} from '../scale';
Expand Down Expand Up @@ -156,10 +156,13 @@ export class UnitModel extends ModelWithField {
if (isFieldDef(channelDef)) {
fieldDef = channelDef;
specifiedScale = channelDef.scale;
} else if (isConditionalDef(channelDef) && isFieldDef(channelDef.condition)) {
fieldDef = channelDef.condition;
specifiedScale = channelDef.condition.scale;
Copy link
Member

Choose a reason for hiding this comment

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

Could these first two branches be consolidated using the new getFieldDef method?

Copy link
Member Author

@kanitw kanitw Jun 9, 2017

Choose a reason for hiding this comment

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

Yeah I tried this earlier too. However, TypeScript won't know that channelDef.condition is not only a FieldDef, but also a ScaleFieldDef. So we either have to do type casting or have to do this and I generally prefer to avoid casting.

} else if (channel === 'x') {
fieldDef = vlEncoding.getFieldDef(encoding, 'x2');
fieldDef = getFieldDef(encoding.x2);
} else if (channel === 'y') {
fieldDef = vlEncoding.getFieldDef(encoding, 'y2');
fieldDef = getFieldDef(encoding.y2);
}

if (fieldDef) {
Expand Down Expand Up @@ -249,12 +252,15 @@ export class UnitModel extends ModelWithField {
private initLegend(encoding: Encoding<string>): Dict<Legend> {
return NONSPATIAL_SCALE_CHANNELS.reduce(function(_legend, channel) {
const channelDef = encoding[channel];
if (isFieldDef(channelDef)) {
const legendSpec = channelDef.legend;
if (legendSpec !== null && legendSpec !== false) {
_legend[channel] = {...legendSpec};
if (channelDef) {
const legend = isFieldDef(channelDef) ? channelDef.legend :
(channelDef.condition && isFieldDef(channelDef.condition)) ? channelDef.condition.legend : null;
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, here, rather than multiple ternaries depending on where the fieldDef might be, could this be simplified with a single getFieldDef call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to #1980 (comment), TS won't know that this is a LegendFieldDef, not just FieldDef.


if (legend !== null && legend !== false) {
_legend[channel] = {...legend};
}
}

return _legend;
}, {});
}
Expand Down
32 changes: 13 additions & 19 deletions src/encoding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,21 @@ import {CompositeAggregate} from './compositemark';
import {Facet} from './facet';
import {
ChannelDef,
Condition,
ConditionalLegendDef,
ConditionalTextDef,
ConditionalValueDef,
Field,
FieldDef,
isFieldDef,
isValueDef,
LegendFieldDef,
normalize,
normalizeFieldDef,
OrderFieldDef,
PositionFieldDef,
TextFieldDef,
PositionDef,
ValueDef
} from './fielddef';
import {Conditional, ConditionalLegendDef, ConditionalTextDef, normalizeFieldDef, PositionDef} from './fielddef';
import {getFieldDef, hasConditionFieldDef} from './fielddef';
import * as log from './log';
import {Mark} from './mark';
import {isArray, some} from './util';
Expand Down Expand Up @@ -110,23 +112,12 @@ export function channelHasField(encoding: EncodingWithFacet<Field>, channel: Cha
if (isArray(channelDef)) {
return some(channelDef, (fieldDef) => !!fieldDef.field);
} else {
return isFieldDef(channelDef);
return isFieldDef(channelDef) || hasConditionFieldDef(channelDef);
}
}
return false;
}

export function getFieldDef<T>(encoding: EncodingWithFacet<T>, channel: Channel): FieldDef<T> {
const channelDef = encoding[channel];
if (isArray(channelDef)) {
throw new Error('getFieldDef should be never used with detail or order when they have multiple fields');
} else if (isFieldDef(channelDef)) {
return channelDef;
}
// TODO: if hasConditionFieldDef

return undefined;
}

export function isAggregate(encoding: EncodingWithFacet<Field>) {
return some(CHANNELS, (channel) => {
Expand All @@ -135,7 +126,8 @@ export function isAggregate(encoding: EncodingWithFacet<Field>) {
if (isArray(channelDef)) {
return some(channelDef, (fieldDef) => !!fieldDef.aggregate);
} else {
return isFieldDef(channelDef) && !!channelDef.aggregate;
const fieldDef = getFieldDef(channelDef);
return fieldDef && !!fieldDef.aggregate;
}
}
return false;
Expand All @@ -153,8 +145,8 @@ export function normalizeEncoding(encoding: Encoding<string>, mark: Mark): Encod

// Drop line's size if the field is aggregated.
if (channel === 'size' && mark === 'line') {
const channelDef = encoding[channel];
if (isFieldDef(channelDef) && channelDef.aggregate) {
const fieldDef = getFieldDef(encoding[channel]);
if (fieldDef && fieldDef.aggregate) {
log.warn(log.message.incompatibleChannel(channel, mark, 'when the field is aggregated.'));
return normalizedEncoding;
}
Expand Down Expand Up @@ -200,6 +192,8 @@ export function fieldDefs(encoding: EncodingWithFacet<Field>): FieldDef<Field>[]
(isArray(channelDef) ? channelDef : [channelDef]).forEach((def) => {
if (isFieldDef(def)) {
arr.push(def);
} else if (hasConditionFieldDef(def)) {
arr.push(def.condition);
}
});
}
Expand Down
39 changes: 34 additions & 5 deletions src/fielddef.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export type Condition<T> = {
* ...
* }
*/
export type ConditionalFieldDef<F, V> = F & {condition?: Condition<V>};
export type ConditionalFieldDef<F extends FieldDef<any>, V extends ValueDef<any>> = F & {condition?: Condition<V>};

/**
* A ValueDef with Condition<ValueDef | FieldDef>
Expand All @@ -55,7 +55,7 @@ export type ConditionalFieldDef<F, V> = F & {condition?: Condition<V>};
* value: ...,
* }
*/
export type ConditionalValueDef<F, V> = V & {condition?: Condition<F | V>};
export type ConditionalValueDef<F extends FieldDef<any>, V extends ValueDef<any>> = V & {condition?: Condition<F | V>};

/**
* Reference to a repeated value.
Expand Down Expand Up @@ -173,7 +173,18 @@ export interface TextFieldDef<F> extends FieldDef<F> {

export type ConditionalTextDef<F> = Conditional<TextFieldDef<F>, ValueDef<string|number|boolean>>;

export type ChannelDef<F> = FieldDef<F> | ValueDef<any>;
export type ChannelDef<F> = Conditional<FieldDef<F>, ValueDef<any>>;

export function isConditionalDef<F>(channelDef: ChannelDef<F>): channelDef is Conditional<FieldDef<F>, ValueDef<any>> {
return !!channelDef && !!channelDef.condition;
}

/**
* Return if a channelDef is a ConditionalValueDef with ConditionFieldDef
*/
export function hasConditionFieldDef<F>(channelDef: ChannelDef<F>): channelDef is (ValueDef<any> & {condition: FieldDef<F>}) {
return !!channelDef && !!channelDef.condition && isFieldDef(channelDef.condition);
}

export function isFieldDef<F>(channelDef: ChannelDef<F>): channelDef is FieldDef<F> | PositionFieldDef<F> | LegendFieldDef<F, any> | OrderFieldDef<F> | TextFieldDef<F> {
return !!channelDef && (!!channelDef['field'] || channelDef['aggregate'] === 'count');
Copy link
Member

Choose a reason for hiding this comment

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

getFieldDef below looks good, but I'm curious why isFieldDef doesn't account for a Conditional field def?

Copy link
Member

Choose a reason for hiding this comment

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

Relatedly, I see that several isFieldDef calls have been replaced with getFieldDef to account for the conditional. If there's a difference in how the two should be used/what they should check for, perhaps we can name them more clearly to reflect that? Unfortunately I don't have suggestions yet because I don't grasp the difference, sorry!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, say a channelDef could be

  1. FieldDef (with no condition)
  2. FieldDef with ConditionValueDef

(1 & 2 are ConditionalFieldDef as I make condition? so I don't have to declare too many types)

  1. ValueDef (with no condition)
  2. ValueDef with ConditionFieldDef
  3. ValueDef with ConditionValueDef

(3-5 are ConditionalValueDef as I make condition? so I don't have to declare too many types)

  1. ConditionFieldDef only (akin to if without else)
  2. ConditionValueDef only

(6-7 are ConditionOnlyDef)

isFieldDef is more scoped to one object and can check if the object is a fieldDef.
The input can be either:

  • a) the parent object of a channelDef (encoding.<channel>), which will return true for 1 or 2,
  • b) the condition object of a channelDef (encoding.<channel>.condition), which will return true for case 4 or 6.

On the other hand, getFieldDef is for getting the fieldDef from a channelDef.
Given a channelDef (from encoding.<channel>), this will return either the parent channelDef object (for case 1 or 2) or its condition object (for case 4 or 6).

In a way, getFieldDef is more like getFieldDefFromChannelDef but it is kinda verbose, so I just chose the brief getFieldDef. That said, I agree that it can be confusing.

If you have better name suggestions, let me know :)

Expand Down Expand Up @@ -296,13 +307,32 @@ export function defaultType(fieldDef: FieldDef<Field>, channel: Channel): Type {
}
}

/**
* Returns the fieldDef -- either from the outer channelDef or from the condition of channelDef.
* @param channelDef
*/
export function getFieldDef<F>(channelDef: ChannelDef<F>): FieldDef<F> {
if (isFieldDef(channelDef)) {
return channelDef;
} else if (hasConditionFieldDef(channelDef)) {
return channelDef.condition;
}
return undefined;
}

/**
* Convert type to full, lowercase type, or augment the fieldDef with a default type if missing.
*/
export function normalize(channelDef: ChannelDef<string>, channel: Channel) {
export function normalize(channelDef: ChannelDef<string>, channel: Channel): ChannelDef<any> {
// If a fieldDef contains a field, we need type.
if (isFieldDef(channelDef)) {
return normalizeFieldDef(channelDef, channel);
} else if (hasConditionFieldDef(channelDef)) {
return {
...channelDef,
// Need to cast as normalizeFieldDef normally return FieldDef, but here we know that it is definitely Condition<FieldDef>
condition: normalizeFieldDef(channelDef.condition, channel) as Condition<FieldDef<string>>
};
}
return channelDef;
}
Expand Down Expand Up @@ -347,7 +377,6 @@ export function normalizeFieldDef(fieldDef: FieldDef<string>, channel: Channel)
log.warn(warning);
}
return fieldDef;

}

export function normalizeBin(bin: Bin|boolean, channel: Channel) {
Expand Down
7 changes: 4 additions & 3 deletions src/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as log from './log';
import {SUM_OPS} from './aggregate';
import {Channel, STACK_GROUP_CHANNELS, X, X2, Y, Y2} from './channel';
import {channelHasField, Encoding, isAggregate} from './encoding';
import {Field, FieldDef, isFieldDef, PositionFieldDef} from './fielddef';
import {Field, FieldDef, getFieldDef, isFieldDef, PositionFieldDef} from './fielddef';
import {AREA, BAR, CIRCLE, isMarkDef, LINE, Mark, MarkDef, POINT, RULE, SQUARE, TEXT, TICK} from './mark';
import {ScaleType} from './scale';
import {contains, isArray} from './util';
Expand Down Expand Up @@ -57,8 +57,9 @@ export function stack(m: Mark | MarkDef, encoding: Encoding<Field>, stackConfig:
const stackBy = STACK_GROUP_CHANNELS.reduce((sc, channel) => {
if (channelHasField(encoding, channel)) {
const channelDef = encoding[channel];
(isArray(channelDef) ? channelDef : [channelDef]).forEach((fieldDef) => {
if (isFieldDef(fieldDef) && !fieldDef.aggregate) {
(isArray(channelDef) ? channelDef : [channelDef]).forEach((cDef) => {
const fieldDef = getFieldDef(cDef);
if (!fieldDef.aggregate) {
sc.push({
channel: channel,
fieldDef: fieldDef
Expand Down