-
Notifications
You must be signed in to change notification settings - Fork 185
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
temporal contours #2254
base: main
Are you sure you want to change the base?
temporal contours #2254
Conversation
1. blur2 does not support dates 2. the contour ticks should be derived as utc ticks not numerical ticks closes #2250
if (typeof thresholds?.range === "function") return thresholds.range(thresholds.floor(min), max); | ||
if (typeof thresholds === "function") thresholds = thresholds(V, min, max); | ||
if (typeof thresholds !== "number") return arrayify(thresholds); | ||
if (temporal) return scaleUtc().domain([min, max]).nice(thresholds).ticks(thresholds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use utcTickInterval
here instead?
if (temporal) {
thresholds = utcTickInterval(min, max, thresholds);
return thresholds.range(thresholds.floor(min), max);
}
Maybe we could promote Eh, this is fine.thresholds
sooner and then you wouldn’t need to pass the temporal
argument down here.
@@ -115,7 +115,8 @@ function contourGeometry({thresholds, interval, ...options}) { | |||
const {pixelSize: k, width: w = Math.round(Math.abs(dx) / k), height: h = Math.round(Math.abs(dy) / k)} = this; | |||
const kx = w / dx; | |||
const ky = h / dy; | |||
const V = channels.value.value; | |||
const temporal = isTemporal(channels.value.value); | |||
const V = temporal && this.blur > 0 ? Float64Array.from(channels.value.value) : channels.value.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use coerceNumbers
here whenever this.blur > 0
, not just in the temporal
case? I also wonder if we could provide a hint earlier when the value channel is declared to say that it must include numbers (at least in the this.blur > 0
case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to make contours work with categorical data, but I feel it would necessitate a completely different approach (we'd have to compute the contours for each of the categories on a boolean raster saying whether a pixel belongs to that category… something like that… with blurring to accomodate the "random-walk" interpolator).
there were two issues:
closes #2250