Skip to content

time intervals coerce isostrings #1326

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

Closed
wants to merge 7 commits into from
Closed
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
8 changes: 8 additions & 0 deletions src/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,11 @@ export function named(things) {
export function maybeNamed(things) {
return isIterable(things) ? named(things) : things;
}

export function isTimeInterval(t) {
return isInterval(t) && typeof t === "function" && t() instanceof Date;
Copy link
Member

Choose a reason for hiding this comment

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

This disagrees with how we’ve defined intervals in TypeScript, where they are objects rather than decorated functions:

plot/src/interval.d.ts

Lines 19 to 22 in 820144e

export interface IntervalImplementation<T> {
floor(value: T): T;
offset(value: T, offset?: number): T;
}

And this means that you can use a {floor, offset, range?} object in Plot, which works, except when we try to do this time interval test.

Also, I don’t think we actually need this restriction, because D3 doesn’t require that time intervals are functions, either. It instead checks for the range method (and then blindly invokes the floor and offset methods):

https://github.com/d3/d3-time/blob/9e8dc940f38f78d7588aad68a54a25b1f0c2d97b/src/ticks.js#L38
https://github.com/d3/d3-scale/blob/2b7db622b1a224d9ea19ff15c4cc8cbb3b25f4a4/src/time.js#L58

I added that duck testing in #572, but I think it would be better to replace that duck testing with something consistent with our new TypeScript declarations and with D3. Probably we should invoke the floor method and see if it returns a Date? But that’s a little messy too, since that method requires an argument, and it’s unclear whether we should pass it a Date or a number… All D3 time intervals support coercion (as does Plot’s internal number interval), but should we expect users to implement coercion in their own intervals? I mean, isn’t the point of this PR is that they don’t? 🤔

}

export function isInterval(t) {
return typeof t?.range === "function";
}
26 changes: 19 additions & 7 deletions src/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,18 @@ import {createLegends, exposeLegends} from "./legends.js";
import {Mark} from "./mark.js";
import {axisFx, axisFy, axisX, axisY, gridFx, gridFy, gridX, gridY} from "./marks/axis.js";
import {frame} from "./marks/frame.js";
import {arrayify, isColor, isIterable, isNone, isScaleOptions, map, yes, maybeInterval} from "./options.js";
import {
arrayify,
coerceDate,
isColor,
isIterable,
isNone,
isScaleOptions,
isTimeInterval,
map,
yes,
maybeInterval
} from "./options.js";
import {createScales, createScaleFunctions, autoScaleRange, exposeScales} from "./scales.js";
import {innerDimensions, outerDimensions} from "./scales.js";
import {position, registry as scaleRegistry} from "./scales/index.js";
Expand Down Expand Up @@ -358,12 +369,13 @@ function applyScaleTransforms(channels, options) {
function applyScaleTransform(channel, options) {
const {scale} = channel;
if (scale == null) return;
const {
type,
percent,
interval,
transform = percent ? (x) => x * 100 : maybeInterval(interval, type)?.floor
} = options[scale] ?? {};
let {type, percent, interval, transform} = options[scale] ?? {};
if (transform === undefined) {
if (percent) return (channel.value = channel.value.map((x) => x * 100)), undefined;
const i = maybeInterval(interval, type);
if (isTimeInterval(i)) return (channel.value = channel.value.map((d) => i.floor(coerceDate(d)))), undefined;
transform = i?.floor;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be cleaner to refactor this into a helper like so:

function maybeIntervalTransform(interval, type) {
  interval = maybeInterval(interval, type);
  return isTimeInterval(interval) ? (d) => interval.floor(coerceDate(d)) : interval?.floor;  
}

And then replace the default like so:

transform = percent ? (x) => x * 100 : maybeIntervalTransform(interval, type)

In other words, I think it’s preferable to consolidate the code paths so that the interval is still promoted to a transform. And also we’d want to use map here instead of channel.value.map because channel.value could be a typed array.

Copy link
Member

Choose a reason for hiding this comment

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

Also do we want to coerce to numbers for number intervals? Should we further assume that intervals only apply to numbers and dates and not other types? 🤔

if (transform != null) channel.value = map(channel.value, transform);
}

Expand Down
10 changes: 2 additions & 8 deletions src/transforms/bin.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
identity,
coerceDate,
coerceNumbers,
isInterval,
isTimeInterval,
maybeColumn,
maybeRangeInterval,
maybeTuple,
Expand Down Expand Up @@ -325,14 +327,6 @@ function isTimeThresholds(t) {
return isTimeInterval(t) || (isIterable(t) && isTemporal(t));
}

function isTimeInterval(t) {
return isInterval(t) && typeof t === "function" && t() instanceof Date;
}

function isInterval(t) {
return typeof t?.range === "function";
}

function bing(EX, EY) {
return EX && EY
? function* (I) {
Expand Down
5 changes: 4 additions & 1 deletion src/transforms/interval.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {coerceDate, isTimeInterval} from "../options.js";
import {isTemporal, labelof, map, maybeInterval, maybeValue, valueof} from "../options.js";
import {maybeInsetX, maybeInsetY} from "./inset.js";

Expand Down Expand Up @@ -28,7 +29,9 @@ function maybeIntervalK(k, maybeInsetK, options, trivial) {
let D1, V1;
function transform(data) {
if (V1 !== undefined && data === D1) return V1; // memoize
return (V1 = map(valueof((D1 = data), value), (v) => interval.floor(v)));
let V = valueof((D1 = data), value);
if (isTimeInterval(interval)) V = map(V, coerceDate, Float64Array);
Copy link
Member

@mbostock mbostock Mar 15, 2023

Choose a reason for hiding this comment

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

Suggested change
if (isTimeInterval(interval)) V = map(V, coerceDate, Float64Array);
if (isTimeInterval(interval)) V = coerceDates(V);

I think we want the slower coerceDates here. This code using Float64Array is faster, but we can only use it in the bin transform because we know it’s not exposed outside the bin transform. I don’t think that’s true here; we want to pass the interval Date instances, not numbers.

Also, given that we can’t use a typed array here, it will probably be faster to fold the coerceDate call into the interval.floor(v) call before to avoid one additional array copy in map.

return (V1 = map(V, (v) => interval.floor(v)));
}
return maybeInsetK({
...options,
Expand Down
548 changes: 548 additions & 0 deletions test/output/dateIntervalWeekMonth.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
642 changes: 642 additions & 0 deletions test/output/downloadsOrdinalIsostrings.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
28 changes: 28 additions & 0 deletions test/plots/date-intervals.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import * as Plot from "@observablehq/plot";
import * as d3 from "d3";

export async function dateIntervalWeekMonth() {
const data = await d3.csv("data/bls-industry-unemployment.csv");
return Plot.plot({
marginLeft: 80,
color: {type: "linear", scheme: "blues"},
marks: (
[
{label: "month", interval: "month"},
{label: "d3.utcMonth", interval: d3.utcMonth},
{label: "week", interval: "week"},
{label: "d3.utcWeek", interval: d3.utcWeek}
] as const
).map(({label, interval}) =>
Plot.barX(data, {
x: "date",
filter: (d) => d.industry === "Construction",
interval,
fill: "unemployed",
title: "unemployed",
inset: 0,
fy: () => label
})
)
});
}
15 changes: 15 additions & 0 deletions test/plots/downloads-ordinal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,18 @@ export async function downloadsOrdinal() {
]
});
}

export async function downloadsOrdinalIsostrings() {
const downloads = (await d3.csv<any>("data/downloads.csv")).filter((d) => /20(19|20)-0/.test(d.date));
return Plot.plot({
width: 960,
marginBottom: 50,
x: {
interval: "month",
tickRotate: -45,
tickFormat: "%b %Y",
label: null
},
marks: [Plot.barY(downloads, {x: "date", y: "downloads", fill: "#ccc", stroke: "currentColor", title: "date"})]
});
}
1 change: 1 addition & 0 deletions test/plots/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export * from "./crimean-war-overlapped.js";
export * from "./crimean-war-stacked.js";
export * from "./d3-survey-2015-comfort.js";
export * from "./d3-survey-2015-why.js";
export * from "./date-intervals.js";
export * from "./darker-dodge.js";
export * from "./decathlon.js";
export * from "./diamonds-boxplot.js";
Expand Down