time intervals coerce isostrings

[Fil]

The discussion below ends up with the recommendation that if we want to do this, we should coerce isostrings to dates in the channels, rather than wrap the interval object.

todo/done:

• isTimeInterval: should follow a better definition of a "time interval", compatible with our typescript def; maybe “floor(0) gives a Date”.
• plot/src/transforms/interval.js (dateIntervalWeekMonth example)
• scales/ordinal.js (scale transform, e.g. regularize dates as ordinal years)
• marks/contour.js (thresholds given as a date interval—ultra rare)

closes #1182

(Note: I considered doing this in d3-time, but it would mean adding d3-time-format as a dependency of d3-time.)

[mbostock]

I think it’s fine to do this for the built-in time intervals, but we shouldn’t do this implicit coercion if someone provides an interval implementation, including one of the D3 time interval implementations. The implicit coercion should be part of the interval implementation, and we can do that for the named time intervals we provide. (The magic of invoking the interval implementation and seeing if it returns a Date is too much, and some intervals may want to have different semantics for input coercion.)

Edit: I mean, I see that we already have “magic” detection for time thresholds in the bin transform. But I think that predates having named time intervals, and it’d probably be better if we didn’t do that, somehow.

[Fil]

I had the same hesitation but went the other way:

• since we're not going to remove the duck-typing of d3 time intervals (for the bin transform), we might as well use it consistently.
• this could help people who use string dates and funky intervals—for example, transitioning from 1 month interval to 2 months interval requires only one operation (switching from "month" to d3.utcMonth.every(2)), instead of that + coercion of strings to dates.

And, it's well defined in the README: “Time intervals are intervals that are also functions that return a Date instance when called with no arguments.”

[mbostock]

This feels awkward to me. Like, isTimeInterval is the way it is because that was a quick and dirty way to test for a D3 time interval. But now we have to recreate that representation—even though we don’t need it—to satisfy the isTimeInterval check. And this is only adding coercion for one of the possible methods (floor) on the time interval, not the main function, not interval.offset and interval.range, etc. I’m not sure what fix I recommend yet.

[mbostock]

What about the other methods, offset, ceil, range?

[Fil]

We can give them the same treatment, but in practice don't use them on user-provided data. Their input is in all cases already coerced.

(Agree it's awkward!)

[mbostock]

I’d still like to try to find a different way to do this. I think in many cases, like with binning, we’ve already done the coercion by the time the interval is applied, so this coercion is redundant. Would it be possible to instead do the coercion outside the interval, before the interval is applied, when the scale interval is a time interval?

Edit: For what it’s worth, the performance of the bin1m isn’t affected by this branch, at least. So performance doesn’t appear to be an issue. Let me think some more.

[mbostock]

What about the other methods, offset, ceil, range?

[mbostock]

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? 🤔

[mbostock]

I’ve surveyed the code base to see where we might need coercion for intervals.

The (implicit) interval transform uses intervals in maybeIntervalK and maybeIntervalMidK. Both of these would benefit from an implicitly-coercing interval. (Though it would also be possible to coerce all the values prior to invoking the interval.)

plot/src/transforms/interval.js

Line 31 in 820144e

return (V1 = map(valueof((D1 = data), value), (v) => interval.floor(v)));

plot/src/transforms/interval.js

Line 50 in 820144e

const V1 = map(valueof(data, value), (v) => interval.floor(v));

A scale transform can be specified as an interval as described in #1182. This happens in applyScaleTransform when the interval.floor is promoted to a transform. This would benefit from an implicitly-coercing interval, too.

plot/src/plot.js

Lines 365 to 367 in 820144e

	transform = percent ? (x) => x * 100 : maybeInterval(interval, type)?.floor
	} = options[scale] ?? {};
	if (transform != null) channel.value = map(channel.value, transform);

The axisMark can also use the scale interval to compute ticks. This would not benefit from an implicitly-coercing interval: the scale should already have coerced values at this point, and we wouldn’t want the interval to use different coercion than the scale.

plot/src/marks/axis.js

Lines 520 to 521 in 820144e

	const [min, max] = extent(scale.domain());
	data = interval.range(min, interval.offset(interval.floor(max))); // inclusive max

Ordinal scales use intervals to compute the domain. This would not benefit from an implicitly-coercing interval: we would need to do the coercion earlier on the scale’s associated channel values, so that the extent is computed correctly and so that the scale’s imputed domain matches the channel values.

plot/src/scales/ordinal.js

Lines 100 to 101 in 820144e

	const [min, max] = extent(values).map(interval.floor, interval);
	return interval.range(min, interval.offset(max));

The contour mark can use an interval to compute thresholds. The majority of the time the contour mark will be used with numeric data, but it is theoretically possible to compute contours of arbitrary values, including temporal data. So, this might benefit from implicit coercion. However, it again would not work to perform that coercion within the interval; we would need to coerce all the contour values, not just coerce when computing ticks.

plot/src/marks/contour.js

Line 191 in 820144e

if (typeof thresholds?.range === "function") return thresholds.range(thresholds.floor(min), max);

The bin mark can likewise use an interval to compute thresholds. It already performs coercion and would not benefit from additional implicit coercion inside the interval.

plot/src/transforms/bin.js

Line 232 in 820144e

V = map(V, coerceDate, Float64Array); // like coerceDates, but faster

To summarize the effect of implicitly-coercing intervals:

• interval transform 👍
• scale transform 👍
• ordinal scales 👎 (incorrect result)
• contour mark 👎 (incorrect result)
• axis mark 👎 (redundant with existing coercion)
• bin transform 👎 (redundant with existing coercion)

My recommendation therefore is, instead of implicitly-coercing time intervals, that we coerce values as appropriate prior to using the interval similar to what the bin transform already does. In the case of ordinal scales, we’ll also need to coerce to numbers, not just dates, so that we compute the extent correctly.

[Fil]

I have revised this PR's description with the TODO.

[mbostock]

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.

[mbostock]

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.

[mbostock]

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? 🤔