Skip to content

Auto mark: never render rect; move zero-ness to autoSpec #1368

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Mar 23, 2023
Merged

Auto mark: never render rect; move zero-ness to autoSpec #1368

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
261594c
Auto mark: never render rect; move zero-ness to autoSpec
tophtucker Mar 21, 2023
f7d100f
update test artifacts
tophtucker Mar 21, 2023
6ac8928
fix some tests; only set zero on a dimension if that dimension is def…
tophtucker Mar 21, 2023
3a375ba
Update src/marks/auto.js
tophtucker Mar 22, 2023
9f5b78d
dont set zero-ness if colorReduce
tophtucker Mar 22, 2023
81b5c17
Merge branch 'toph/never-rect' of https://github.com/observablehq/plo…
tophtucker Mar 22, 2023
2907f2f
fix some tests
tophtucker Mar 22, 2023
fcce38b
prettier
tophtucker Mar 22, 2023
0b3efd3
just committing the state after pairing so i have it
tophtucker Mar 22, 2023
31ba146
revert auto file
tophtucker Mar 22, 2023
455aeeb
re-fix the original motivating bugs, i think
tophtucker Mar 22, 2023
4c970d0
autoImpl
tophtucker Mar 23, 2023
1ab57ea
rm autoplot matrix test bc it didnt work with test runner
tophtucker Mar 23, 2023
d66d501
transformImpl; coerce zero; sort imports; const
mbostock Mar 23, 2023
d5b1339
normalize mark option
mbostock Mar 23, 2023
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
197 changes: 101 additions & 96 deletions src/marks/auto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,35 @@
import {ascending, InternSet} from "d3";
import {isOrdinal, labelof, valueof, isOptions, isColor, isObject} from "../options.js";
import {marks} from "../mark.js";
import {isColor, isObject, isOptions, isOrdinal, labelof, valueof} from "../options.js";
import {bin, binX, binY} from "../transforms/bin.js";
import {group, groupX, groupY} from "../transforms/group.js";
import {areaX, areaY} from "./area.js";
import {dot} from "./dot.js";
import {line, lineX, lineY} from "./line.js";
import {ruleX, ruleY} from "./rule.js";
import {barX, barY} from "./bar.js";
import {rect, rectX, rectY} from "./rect.js";
import {cell} from "./cell.js";
import {dot} from "./dot.js";
import {frame} from "./frame.js";
import {bin, binX, binY} from "../transforms/bin.js";
import {group, groupX, groupY} from "../transforms/group.js";
import {marks} from "../mark.js";
import {line, lineX, lineY} from "./line.js";
import {rectX, rectY} from "./rect.js";
import {ruleX, ruleY} from "./rule.js";

export function autoSpec(data, options) {
const {x, y, fx, fy, color, size, mark} = autoImpl(data, options);
return {x, y, fx, fy, color, size, mark};
}

function autoImpl(data, options) {
options = normalizeOptions(options);

// Greedily materialize columns for type inference; we’ll need them anyway to
// plot! Note that we don’t apply any type inference to the fx and fy
// channels, if present; these are always ordinal (at least for now).
const {x, y, color, size} = options;
const X = materializeValue(data, x);
const Y = materializeValue(data, y);
const C = materializeValue(data, color);
const S = materializeValue(data, size);
Comment on lines +23 to +30
Copy link
Contributor Author

@tophtucker tophtucker Mar 23, 2023

Choose a reason for hiding this comment

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

moved here from top of auto, which it was passing into autoSpec

Copy link
Member

@mbostock mbostock Mar 23, 2023

Choose a reason for hiding this comment

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

I think we still want to do this materialization (also) inside of auto, so that it doesn’t need to be done twice (the second time being when we call auto.plot).

Copy link
Contributor Author

@tophtucker tophtucker Mar 23, 2023

Choose a reason for hiding this comment

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

Ooh right, we're not passing these materialized values back out of autoImpl… and we don't want to, because autoSpec shouldn't return the materialized values. So I guess auto should greedily materialize and autoImpl shouldn't bc autoSpec should return something cleaner if possible?

Copy link
Member

@mbostock mbostock Mar 23, 2023

Choose a reason for hiding this comment

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

I was wrong; it doesn’t materialize twice because autoImpl returns the full markOptions with the already-materialized values. I think we’re good as-is. Nice!


// Compute the default options.
let {
fx,
fy,
Expand All @@ -25,10 +40,6 @@ export function autoSpec(data, options) {
mark
} = options;

// Lazily materialize x and y columns for type inference, if needed.
const {x, y} = options;
let X, Y;

// Determine the default reducer, if any.
if (xReduce === undefined)
xReduce = yReduce == null && xValue == null && sizeValue == null && yValue != null ? "count" : null;
Expand All @@ -42,8 +53,8 @@ export function autoSpec(data, options) {
colorReduce == null &&
xReduce == null &&
yReduce == null &&
(xValue == null || isOrdinal((X ??= materializeValue(data, x)))) &&
(yValue == null || isOrdinal((Y ??= materializeValue(data, y))))
(xValue == null || isOrdinal(X)) &&
(yValue == null || isOrdinal(Y))
) {
sizeReduce = "count";
}
Expand All @@ -62,102 +73,44 @@ export function autoSpec(data, options) {
mark =
sizeValue != null || sizeReduce != null
? "dot"
: xZero || yZero || colorReduce != null // histogram or heatmap
: isZeroReducer(xReduce) || isZeroReducer(yReduce) || colorReduce != null // histogram or heatmap
? "bar"
: xValue != null && yValue != null
? isOrdinal((X ??= materializeValue(data, x))) ||
isOrdinal((Y ??= materializeValue(data, y))) ||
(xReduce == null && yReduce == null && !isMonotonic(X) && !isMonotonic(Y))
? isOrdinal(X) || isOrdinal(Y) || (xReduce == null && yReduce == null && !isMonotonic(X) && !isMonotonic(Y))
? "dot"
: "line"
: xValue != null || yValue != null
? "rule"
: null;
}

return {
fx: fx ?? null,
fy: fy ?? null,
x: {
value: xValue ?? null,
reduce: xReduce ?? null,
...(xZero !== undefined && {zero: xZero}), // TODO realize default
...xOptions
},
y: {
value: yValue ?? null,
reduce: yReduce ?? null,
...(yZero !== undefined && {zero: yZero}), // TODO realize default
...yOptions
},
color: {
value: colorValue ?? null,
reduce: colorReduce ?? null,
...(colorColor !== undefined && {color: colorColor})
},
size: {
value: sizeValue ?? null,
reduce: sizeReduce ?? null
},
mark
};
}

export function auto(data, options) {
options = normalizeOptions(options);

// Greedily materialize columns for type inference; we’ll need them anyway to
// plot! Note that we don’t apply any type inference to the fx and fy
// channels, if present; these are always ordinal (at least for now).
const {x, y, color, size} = options;
const X = materializeValue(data, x);
const Y = materializeValue(data, y);
const C = materializeValue(data, color);
const S = materializeValue(data, size);

// Compute the default options via autoSpec.
let {
fx,
fy,
x: {reduce: xReduce, zero: xZero, ...xOptions},
y: {reduce: yReduce, zero: yZero, ...yOptions},
color: {color: colorColor, reduce: colorReduce},
size: {reduce: sizeReduce},
mark
} = autoSpec(data, {
...options,
x: {...x, value: X},
y: {...y, value: Y},
color: {...color, value: C},
size: {...size, value: S}
});

let Z; // may be set to null to disable series-by-color for line and area
let colorMode; // "fill" or "stroke"

// Determine the mark implementation.
let markImpl;
if (mark != null) {
switch (`${mark}`.toLowerCase()) {
case "dot":
mark = dot;
markImpl = dot;
colorMode = "stroke";
break;
case "line":
mark = X && Y ? line : X ? lineX : lineY; // 1d line by index
markImpl = X && Y ? line : X ? lineX : lineY; // 1d line by index
colorMode = "stroke";
if (isHighCardinality(C)) Z = null; // TODO only if z not set by user
break;
case "area":
mark = yZero ? areaY : xZero || (Y && isMonotonic(Y)) ? areaX : areaY; // favor areaY if unsure
markImpl = yZero ? areaY : xZero || (Y && isMonotonic(Y)) ? areaX : areaY; // favor areaY if unsure
colorMode = "fill";
if (isHighCardinality(C)) Z = null; // TODO only if z not set by user
break;
case "rule":
mark = X ? ruleX : ruleY;
markImpl = X ? ruleX : ruleY;
colorMode = "stroke";
break;
case "bar":
mark = yZero
markImpl = yZero
? isOrdinalReduced(xReduce, X)
? barY
: rectY
Expand All @@ -171,7 +124,7 @@ export function auto(data, options) {
? barY
: isOrdinalReduced(yReduce, Y)
? barX
: rect;
: rectY;
colorMode = "fill";
break;
default:
Expand All @@ -189,44 +142,95 @@ export function auto(data, options) {
z: Z,
r: S ?? undefined // treat null size as undefined for default constant radius
};
let transform;
let transformImpl;
let transformOptions = {[colorMode]: colorReduce ?? undefined, r: sizeReduce ?? undefined};
if (xReduce != null && yReduce != null) {
throw new Error(`cannot reduce both x and y`); // for now at least
} else if (yReduce != null) {
transformOptions.y = yReduce;
transform = isOrdinal(X) ? groupX : binX;
transformImpl = isOrdinal(X) ? groupX : binX;
} else if (xReduce != null) {
transformOptions.x = xReduce;
transform = isOrdinal(Y) ? groupY : binY;
transformImpl = isOrdinal(Y) ? groupY : binY;
} else if (colorReduce != null || sizeReduce != null) {
if (X && Y) {
transform = isOrdinal(X) && isOrdinal(Y) ? group : isOrdinal(X) ? binY : isOrdinal(Y) ? binX : bin;
transformImpl = isOrdinal(X) && isOrdinal(Y) ? group : isOrdinal(X) ? binY : isOrdinal(Y) ? binX : bin;
} else if (X) {
transform = isOrdinal(X) ? groupX : binX;
transformImpl = isOrdinal(X) ? groupX : binX;
} else if (Y) {
transform = isOrdinal(Y) ? groupY : binY;
transformImpl = isOrdinal(Y) ? groupY : binY;
}
}
if (transform) {
if (transform === bin || transform === binX) markOptions.x = {value: X, ...xOptions};
if (transform === bin || transform === binY) markOptions.y = {value: Y, ...yOptions};
markOptions = transform(transformOptions, markOptions);
Copy link
Contributor Author

@tophtucker tophtucker Mar 23, 2023

Choose a reason for hiding this comment

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

Moved down into auto bc it's instantiating

}

// When using the bin transform, pass through additional options (e.g., thresholds).
if (transformImpl === bin || transformImpl === binX) markOptions.x = {value: X, ...xOptions};
if (transformImpl === bin || transformImpl === binY) markOptions.y = {value: Y, ...yOptions};

// If zero-ness is not specified, default based on whether the resolved mark
// type will include a zero baseline. TODO Move this to autoSpec.
// type will include a zero baseline.
if (xZero === undefined)
xZero = X && transform !== binX && (mark === barX || mark === areaX || mark === rectX || mark === ruleY);
xZero =
X &&
!(transformImpl === bin || transformImpl === binX) &&
(markImpl === barX || markImpl === areaX || markImpl === rectX || markImpl === ruleY);
if (yZero === undefined)
yZero = Y && transform !== binY && (mark === barY || mark === areaY || mark === rectY || mark === ruleX);
yZero =
Y &&
!(transformImpl === bin || transformImpl === binY) &&
(markImpl === barY || markImpl === areaY || markImpl === rectY || markImpl === ruleX);

return {
fx: fx ?? null,
fy: fy ?? null,
x: {
value: xValue ?? null,
reduce: xReduce ?? null,
zero: !!xZero,
...xOptions
},
y: {
value: yValue ?? null,
reduce: yReduce ?? null,
zero: !!yZero,
...yOptions
},
color: {
value: colorValue ?? null,
reduce: colorReduce ?? null,
...(colorColor !== undefined && {color: colorColor})
},
size: {
value: sizeValue ?? null,
reduce: sizeReduce ?? null
},
mark,
markImpl,
markOptions,
transformImpl,
transformOptions,
colorMode
};
}

export function auto(data, options) {
const {
fx,
fy,
x: {zero: xZero},
y: {zero: yZero},
markImpl,
markOptions,
transformImpl,
transformOptions,
colorMode
} = autoImpl(data, options);

// In the case of filled marks (particularly bars and areas) the frame and
// rules should come after the mark; in the case of stroked marks
// (particularly dots and lines) they should come before the mark.
const frames = fx != null || fy != null ? frame({strokeOpacity: 0.1}) : null;
const rules = [xZero ? ruleX([0]) : null, yZero ? ruleY([0]) : null];
mark = mark(data, markOptions);
const mark = markImpl(data, transformImpl ? transformImpl(transformOptions, markOptions) : markOptions);
return colorMode === "stroke" ? marks(frames, rules, mark) : marks(frames, mark, rules);
}

Expand All @@ -253,6 +257,7 @@ function isMonotonic(values) {
// (but note that they can also be specified as a {transform} object such as
// Plot.identity). We don’t support reducers for the faceting, but for symmetry
// with x and y we allow facets to be specified as {value} objects.
// TODO Normalize mark name to lowercase?
function normalizeOptions({x, y, color, size, fx, fy, mark} = {}) {
if (!isOptions(x)) x = makeOptions(x);
if (!isOptions(y)) y = makeOptions(y);
Expand Down
16 changes: 8 additions & 8 deletions test/marks/auto-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ it("Plot.autoSpec makes a histogram from a quantitative dimension", () => {
assert.deepStrictEqual(Plot.autoSpec(data, {x: "value"}), {
fx: null,
fy: null,
x: {value: "value", reduce: null},
x: {value: "value", reduce: null, zero: false},
y: {value: null, reduce: "count", zero: true},
color: {value: null, reduce: null},
size: {value: null, reduce: null},
Expand All @@ -19,7 +19,7 @@ it("Plot.autoSpec makes a bar chart from an ordinal dimension", () => {
assert.deepStrictEqual(Plot.autoSpec(data, {x: "value", color: "blue"}), {
fx: null,
fy: null,
x: {value: "value", reduce: null},
x: {value: "value", reduce: null, zero: false},
y: {value: null, reduce: "count", zero: true},
color: {value: null, reduce: null, color: "blue"},
size: {value: null, reduce: null},
Expand All @@ -36,8 +36,8 @@ it("Plot.autoSpec makes a line from a monotonic dimension", () => {
assert.deepStrictEqual(Plot.autoSpec(data, {x: "date", y: "value"}), {
fx: null,
fy: null,
x: {value: "date", reduce: null},
y: {value: "value", reduce: null},
x: {value: "date", reduce: null, zero: false},
y: {value: "value", reduce: null, zero: false},
color: {value: null, reduce: null},
size: {value: null, reduce: null},
mark: "line"
Expand All @@ -53,8 +53,8 @@ it("Plot.autoSpec makes a dot plot from two quantitative dimensions", () => {
assert.deepStrictEqual(Plot.autoSpec(data, {x: "x", y: "y"}), {
fx: null,
fy: null,
x: {value: "x", reduce: null},
y: {value: "y", reduce: null},
x: {value: "x", reduce: null, zero: false},
y: {value: "y", reduce: null, zero: false},
color: {value: null, reduce: null},
size: {value: null, reduce: null},
mark: "dot"
Expand All @@ -73,8 +73,8 @@ it("Plot.autoSpec makes a faceted heatmap", () => {
assert.deepStrictEqual(Plot.autoSpec(data, {x: "x", y: "y", fy: "f", color: "count"}), {
fx: null,
fy: "f",
x: {value: "x", reduce: null},
y: {value: "y", reduce: null},
x: {value: "x", reduce: null, zero: false},
y: {value: "y", reduce: null, zero: false},
color: {value: null, reduce: "count"},
size: {value: null, reduce: null},
mark: "bar"
Expand Down
Loading