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

fix: support discretizing scales for shape #7170

Merged
merged 2 commits into from
Jan 31, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
26 changes: 9 additions & 17 deletions src/channeldef.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ import * as log from './log';
import {LogicalComposition} from './logical';
import {isRectBasedMark, Mark, MarkDef} from './mark';
import {Predicate} from './predicate';
import {Scale, SCALE_CATEGORY_INDEX} from './scale';
import {isContinuousToDiscrete, Scale, SCALE_CATEGORY_INDEX} from './scale';
import {isSortByChannel, Sort, SortOrder} from './sort';
import {isFacetFieldDef} from './spec/facet';
import {StackOffset, StackProperties} from './stack';
Expand Down Expand Up @@ -657,7 +657,7 @@ export function isContinuousFieldOrDatumDef<F extends Field>(
cd: ChannelDef<F>
): cd is TypedFieldDef<F> | DatumDef<F, number> {
// TODO: make datum support DateTime object
return (isTypedFieldDef(cd) && isContinuous(cd)) || isNumericDataDef(cd);
return (isTypedFieldDef(cd) && !isDiscrete(cd)) || isNumericDataDef(cd);
}

export function isQuantitativeFieldOrDatumDef<F extends Field>(cd: ChannelDef<F>) {
Expand Down Expand Up @@ -815,8 +815,8 @@ export function isDiscrete(def: TypedFieldDef<Field> | DatumDef<any, any>) {
throw new Error(log.message.invalidFieldType(def.type));
}

export function isContinuous(fieldDef: TypedFieldDef<Field>) {
return !isDiscrete(fieldDef);
export function isDiscretizing(def: TypedFieldDef<Field> | DatumDef<any, any>) {
return isScaleFieldDef(def) && isContinuousToDiscrete(def.scale?.type);
}

export function isCount(fieldDef: FieldDefBase<Field>) {
Expand Down Expand Up @@ -1204,10 +1204,10 @@ export function channelCompatibility(
case ROW:
case COLUMN:
case FACET:
if (isContinuous(fieldDef)) {
if (!isDiscrete(fieldDef)) {
return {
compatible: false,
warning: log.message.facetChannelShouldBeDiscrete(channel)
warning: log.message.channelShouldBeDiscrete(channel)
};
}
return COMPATIBLE;
Expand Down Expand Up @@ -1258,20 +1258,12 @@ export function channelCompatibility(
}
return COMPATIBLE;

case STROKEDASH:
if (!['ordinal', 'nominal'].includes(fieldDef.type)) {
return {
compatible: false,
warning: 'StrokeDash channel should be used with only discrete data.'
};
}
return COMPATIBLE;

case SHAPE:
if (!['ordinal', 'nominal', 'geojson'].includes(fieldDef.type)) {
case STROKEDASH:
if (!isDiscrete(fieldDef) && !isDiscretizing(fieldDef)) {
return {
compatible: false,
warning: 'Shape channel should be used with only either discrete or geojson data.'
warning: log.message.channelShouldBeDiscreteOrDiscretizing(channel)
};
}
return COMPATIBLE;
Expand Down
6 changes: 5 additions & 1 deletion src/log/message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,14 @@ export function invalidEncodingChannel(channel: ExtendedChannel) {
return `${channel}-encoding is dropped as ${channel} is not a valid encoding channel.`;
}

export function facetChannelShouldBeDiscrete(channel: FacetChannel) {
export function channelShouldBeDiscrete(channel: ExtendedChannel) {
return `${channel} encoding should be discrete (ordinal / nominal / binned).`;
}

export function channelShouldBeDiscreteOrDiscretizing(channel: ExtendedChannel) {
return `${channel} encoding should be discrete (ordinal / nominal / binned) or use a discretizing scale (e.g. threshold).`;
}

export function facetChannelDropped(channels: FacetChannel[]) {
return `Facet encoding dropped as ${channels.join(' and ')} ${channels.length > 1 ? 'are' : 'is'} also specified.`;
}
Expand Down
5 changes: 2 additions & 3 deletions src/scale.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const SCALE_CATEGORY_INDEX: Record<ScaleType, ScaleType | 'numeric' | 'or
threshold: 'discretizing'
};

export const SCALE_TYPES = keys(SCALE_CATEGORY_INDEX) as ScaleType[];
export const SCALE_TYPES: ScaleType[] = keys(SCALE_CATEGORY_INDEX);

/**
* Whether the two given scale types can be merged together.
Expand Down Expand Up @@ -839,8 +839,7 @@ export function channelSupportScaleType(channel: Channel, scaleType: ScaleType):
case CHANNEL.STROKE:
return scaleType !== 'band'; // band does not make sense with color
case CHANNEL.STROKEDASH:
return scaleType === 'ordinal' || isContinuousToDiscrete(scaleType);
case CHANNEL.SHAPE:
return scaleType === 'ordinal'; // shape = lookup only
return scaleType === 'ordinal' || isContinuousToDiscrete(scaleType);
}
}
2 changes: 1 addition & 1 deletion test/compile/facet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('FacetModel', () => {
}
});
expect(model.facet).toEqual({row: {field: 'a', type: 'quantitative'}});
expect(localLogger.warns[0]).toEqual(log.message.facetChannelShouldBeDiscrete(ROW));
expect(localLogger.warns[0]).toEqual(log.message.channelShouldBeDiscrete(ROW));
})
);

Expand Down
30 changes: 25 additions & 5 deletions test/scale.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import {ScaleChannel, SCALE_CHANNELS} from '../src/channel';
import * as scale from '../src/scale';
import {channelSupportScaleType, CONTINUOUS_TO_CONTINUOUS_SCALES, ScaleType, SCALE_TYPES} from '../src/scale';
import {
channelSupportScaleType,
CONTINUOUS_TO_CONTINUOUS_SCALES,
CONTINUOUS_TO_DISCRETE_SCALES,
ScaleType,
SCALE_TYPES
} from '../src/scale';
import {some} from '../src/util';
import {without} from './util';

Expand Down Expand Up @@ -51,14 +57,28 @@ describe('scale', () => {
}
});

it('shape should support only ordinal', () => {
expect(channelSupportScaleType('shape', 'ordinal')).toBeTruthy();
const nonOrdinal = without<ScaleType>(SCALE_TYPES, ['ordinal']);
for (const scaleType of nonOrdinal) {
it('shape should support only ordinal and discretizing scales', () => {
const supportedScales = [ScaleType.ORDINAL, ...CONTINUOUS_TO_DISCRETE_SCALES] as const;
for (const scaleType of supportedScales) {
expect(channelSupportScaleType('shape', scaleType)).toBeTruthy();
}
const unsupported = without(SCALE_TYPES, supportedScales);
for (const scaleType of unsupported) {
expect(!channelSupportScaleType('shape', scaleType)).toBeTruthy();
}
});

it('strokeDash should support only ordinal and discretizing scales', () => {
const supportedScales = [ScaleType.ORDINAL, ...CONTINUOUS_TO_DISCRETE_SCALES] as const;
for (const scaleType of supportedScales) {
expect(channelSupportScaleType('strokeDash', scaleType)).toBeTruthy();
}
const unsupported = without(SCALE_TYPES, supportedScales);
for (const scaleType of unsupported) {
expect(!channelSupportScaleType('strokeDash', scaleType)).toBeTruthy();
}
});

it('color should support all scale types except band', () => {
for (const scaleType of SCALE_TYPES) {
expect(channelSupportScaleType('color', scaleType)).toEqual(scaleType !== 'band');
Expand Down