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: make tick support relative band size (so it works better with binned fields) #9176

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
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
14 changes: 10 additions & 4 deletions src/compile/mark/encode/position-rect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export function rectPosition(model: UnitModel, channel: 'x' | 'y' | 'theta' | 'r

const offsetScaleChannel = getOffsetChannel(channel);

const isBarBand = mark === 'bar' && (channel === 'x' ? orient === 'vertical' : orient === 'horizontal');
const isBandChannel =
(mark === 'bar' && (channel === 'x' ? orient === 'vertical' : orient === 'horizontal')) ||
(mark === 'tick' && (channel === 'x' ? orient === 'horizontal' : orient === 'vertical'));

// x, x2, and width -- we must specify two of these in all conditions
if (
Expand All @@ -66,7 +68,7 @@ export function rectPosition(model: UnitModel, channel: 'x' | 'y' | 'theta' | 'r
channel,
model
});
} else if (((isFieldOrDatumDef(channelDef) && hasDiscreteDomain(scaleType)) || isBarBand) && !channelDef2) {
} else if (((isFieldOrDatumDef(channelDef) && hasDiscreteDomain(scaleType)) || isBandChannel) && !channelDef2) {
return positionAndSize(channelDef, channel, model);
} else {
return rangePosition(channel, model, {defaultPos: 'zeroOrMax', defaultPos2: 'zeroOrMin'});
Expand Down Expand Up @@ -136,7 +138,7 @@ function positionAndSize(
channel: 'x' | 'y' | 'theta' | 'radius',
model: UnitModel
) {
const {markDef, encoding, config, stack} = model;
const {mark, markDef, encoding, config, stack} = model;
const orient = markDef.orient;

const scaleName = model.scaleName(channel);
Expand All @@ -149,7 +151,11 @@ function positionAndSize(
const offsetScale = model.getScaleComponent(getOffsetScaleChannel(channel));

// use "size" channel for bars, if there is orient and the channel matches the right orientation
const useVlSizeChannel = (orient === 'horizontal' && channel === 'y') || (orient === 'vertical' && channel === 'x');
const useVlSizeChannel =
mark === 'tick'
? // tick's orientation is opposite to other marks
(orient === 'vertical' && channel === 'y') || (orient === 'horizontal' && channel === 'x')
: (orient === 'horizontal' && channel === 'y') || (orient === 'vertical' && channel === 'x');

// Use size encoding / mark property / config if it exists
let sizeMixins;
Expand Down
53 changes: 15 additions & 38 deletions src/compile/mark/tick.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

This file has the core change to use positionRect with tick instead.

Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
import type {SignalRef} from 'vega';
import {isNumber} from 'vega-util';
import {getViewConfigDiscreteStep} from '../../config';
import {isVgRangeStep} from '../../vega.schema';
import {getMarkPropOrConfig, signalOrValueRef} from '../common';
import {UnitModel} from '../unit';
import {MarkCompiler} from './base';
Expand All @@ -14,10 +10,9 @@ export const tick: MarkCompiler = {
const {config, markDef} = model;
const orient = markDef.orient;

const vgSizeChannel = orient === 'horizontal' ? 'width' : 'height';
const vgThicknessChannel = orient === 'horizontal' ? 'height' : 'width';

return {
const baseAndThickness = {
...encode.baseEncodeEntry(model, {
align: 'ignore',
baseline: 'ignore',
Expand All @@ -26,40 +21,22 @@ export const tick: MarkCompiler = {
size: 'ignore',
theta: 'ignore'
}),

...encode.pointPosition('x', model, {defaultPos: 'mid', vgChannel: 'xc'}),
...encode.pointPosition('y', model, {defaultPos: 'mid', vgChannel: 'yc'}),

// size / thickness => width / height
...encode.nonPosition('size', model, {
defaultValue: defaultSize(model),
vgChannel: vgSizeChannel
}),
[vgThicknessChannel]: signalOrValueRef(getMarkPropOrConfig('thickness', markDef, config))
};
}
};

function defaultSize(model: UnitModel): number | SignalRef {
const {config, markDef} = model;
const {orient} = markDef;

const vgSizeChannel = orient === 'horizontal' ? 'width' : 'height';
const scale = model.getScaleComponent(orient === 'horizontal' ? 'x' : 'y');

const markPropOrConfig =
getMarkPropOrConfig('size', markDef, config, {vgChannel: vgSizeChannel}) ?? config.tick.bandSize;

if (markPropOrConfig !== undefined) {
return markPropOrConfig;
} else {
const scaleRange = scale ? scale.get('range') : undefined;
if (scaleRange && isVgRangeStep(scaleRange) && isNumber(scaleRange.step)) {
return (scaleRange.step * 3) / 4;
if (orient === 'horizontal') {
return {
...baseAndThickness,
...encode.rectPosition(model, 'x'),
...encode.pointPosition('y', model, {defaultPos: 'mid', vgChannel: 'yc'})
};
} else {
// vertical
return {
...baseAndThickness,
...encode.pointPosition('x', model, {defaultPos: 'mid', vgChannel: 'xc'}),
...encode.rectPosition(model, 'y')
};
}

const defaultViewStep = getViewConfigDiscreteStep(config.view, vgSizeChannel);

return (defaultViewStep * 3) / 4;
}
}
};
2 changes: 1 addition & 1 deletion src/compile/scale/type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ function defaultType(
}

if (isXorY(channel) || isXorYOffset(channel)) {
if (util.contains(['rect', 'bar', 'image', 'rule'], mark.type)) {
if (util.contains(['rect', 'bar', 'image', 'tick', 'rule'], mark.type)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to make tick use band scale by default, just like bar/rect, to leverage the same set of logic.

// The rect/bar mark should fit into a band.
// For rule, using band scale to make rule align with axis ticks better https://github.com/vega/vega-lite/issues/3429
return 'band';
Expand Down
51 changes: 33 additions & 18 deletions src/mark.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export function isPathMark(m: Mark | CompositeMark): m is 'line' | 'area' | 'tra
}

export function isRectBasedMark(m: Mark | CompositeMark): m is 'rect' | 'bar' | 'image' | 'arc' {
return ['rect', 'bar', 'image', 'arc' /* arc is rect/interval in polar coordinate */].includes(m);
return ['rect', 'bar', 'image', 'arc', 'tick' /* arc is rect/interval in polar coordinate */].includes(m);
}

export const PRIMITIVE_MARKS = new Set(keys(Mark));
Expand Down Expand Up @@ -284,17 +284,6 @@ export interface MarkConfig<ES extends ExprRef | SignalRef>
outerRadius?: number | ES;
}

export interface RectBinSpacingMixins {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is blended with BandSizeConfigMixins below

/**
* Offset between bars for binned field. The ideal value for this is either 0 (preferred by statisticians) or 1 (Vega-Lite default, D3 example style).
*
* __Default value:__ `1`
*
* @minimum 0
*/
binSpacing?: number;
}

export type AnyMark = CompositeMark | CompositeMarkDef | Mark | MarkDef;

export function isMarkDef(mark: string | GenericMarkDef<any>): mark is GenericMarkDef<any> {
Expand Down Expand Up @@ -333,14 +322,23 @@ const VL_ONLY_MARK_CONFIG_INDEX: Flag<keyof VLOnlyMarkConfig<any>> = {

export const VL_ONLY_MARK_CONFIG_PROPERTIES = keys(VL_ONLY_MARK_CONFIG_INDEX);

const VL_ONLY_BAND_SIZE_CONFIG_MIXINS_INDEX: Flag<keyof BandSizeConfigMixins<any>> = {
binSpacing: 1,
continuousBandSize: 1,
discreteBandSize: 1,
minBandSize: 1
};

const VL_ONLY_BAND_SIZE_CONFIG_MIXINS_PROPS = keys(VL_ONLY_BAND_SIZE_CONFIG_MIXINS_INDEX);

export const VL_ONLY_MARK_SPECIFIC_CONFIG_PROPERTY_INDEX: {
[k in Mark]?: (keyof Required<MarkConfigMixins<any>>[k])[];
} = {
area: ['line', 'point'],
bar: ['binSpacing', 'continuousBandSize', 'discreteBandSize', 'minBandSize'],
rect: ['binSpacing', 'continuousBandSize', 'discreteBandSize', 'minBandSize'],
bar: VL_ONLY_BAND_SIZE_CONFIG_MIXINS_PROPS,
rect: VL_ONLY_BAND_SIZE_CONFIG_MIXINS_PROPS,
line: ['point'],
tick: ['bandSize', 'thickness']
tick: ['bandSize', 'thickness', ...VL_ONLY_BAND_SIZE_CONFIG_MIXINS_PROPS]
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, we need to add this, so we don't output these properties in the output Vega config.

};

export const defaultMarkConfig: MarkConfig<SignalRef> = {
Expand Down Expand Up @@ -427,7 +425,9 @@ const MARK_CONFIG_INDEX: Flag<keyof MarkConfigMixins<any>> = {

export const MARK_CONFIGS = keys(MARK_CONFIG_INDEX);

export interface RectConfig<ES extends ExprRef | SignalRef> extends RectBinSpacingMixins, MarkConfig<ES> {
export type RectConfig<ES extends ExprRef | SignalRef> = MarkConfig<ES> & BandSizeConfigMixins<ES>;

interface BandSizeConfigMixins<ES extends ExprRef | SignalRef> {
/**
* The default size of the bars on continuous scales.
*
Expand All @@ -448,6 +448,15 @@ export interface RectConfig<ES extends ExprRef | SignalRef> extends RectBinSpaci
* __Default value:__ `0.25`
*/
minBandSize?: number | ES;

/**
* Offset between bars for binned field. The ideal value for this is either 0 (preferred by statisticians) or 1 (Vega-Lite default, D3 example style).
*
* __Default value:__ `1`
*
* @minimum 0
*/
binSpacing?: number;
}

export type BandSize = number | RelativeBandSize | SignalRef;
Expand Down Expand Up @@ -667,7 +676,10 @@ export const defaultRectConfig: RectConfig<SignalRef> = {
timeUnitBandPosition: 0.5
};

export interface TickConfig<ES extends ExprRef | SignalRef> extends MarkConfig<ES>, TickThicknessMixins {
export interface TickConfig<ES extends ExprRef | SignalRef>
extends MarkConfig<ES>,
TickThicknessMixins,
BandSizeConfigMixins<ES> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tick now has BandSizeConfigMixins, because we're making tick more adaptable to band + support relative band size, by using rectPosition.

/**
* The width of the ticks.
*
Expand All @@ -678,7 +690,10 @@ export interface TickConfig<ES extends ExprRef | SignalRef> extends MarkConfig<E
}

export const defaultTickConfig: TickConfig<SignalRef> = {
thickness: 1
thickness: 1,
discreteBandSize: {band: 0.75},
Copy link
Member Author

Choose a reason for hiding this comment

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

image

0.75 here are mainly to match 3/4 multiplier in the old code:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The two screenshots look the same to me.

timeUnitBandPosition: 0.5,
timeUnitBandSize: 0.75
};

export function getMarkType(m: string | GenericMarkDef<any>) {
Expand Down
4 changes: 2 additions & 2 deletions test/compile/mark/tick.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,15 @@ describe('Mark: Tick', () => {
});

it('should scale on y', () => {
expect(props.yc).toEqual({scale: Y, field: 'Cylinders'});
expect(props.y).toEqual({scale: Y, field: 'Cylinders', band: 0.125});
});

it('width should be tick thickness with default orient vertical', () => {
expect(props.width).toEqual({value: 1});
});

it('height should be matched to field with default orient vertical', () => {
expect(props.height).toEqual({value: 15});
expect(props.height).toEqual({signal: "0.75 * bandwidth('y')"});
});
});

Expand Down
2 changes: 1 addition & 1 deletion test/compile/scale/type.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('compile/scale', () => {
describe('continuous', () => {
it('should return point scale for ordinal X,Y for marks others than rect, rule, bar, and arc', () => {
PRIMITIVE_MARKS.forEach(mark => {
if (util.contains(['bar', 'rule', 'rect', 'image', 'arc'], mark)) {
if (util.contains(['bar', 'rule', 'rect', 'image', 'arc', 'tick'], mark)) {
return;
}

Expand Down