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

temporal contours #2254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

temporal contours #2254

wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Nov 25, 2024

there were two issues:

  1. blur2 does not support dates
  2. the contour ticks should be derived as utc ticks not numerical ticks

closes #2250

1. blur2 does not support dates
2. the contour ticks should be derived as utc ticks not numerical ticks

closes #2250
@Fil Fil requested a review from mbostock November 25, 2024 14:12
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);
Copy link
Member

@mbostock mbostock Nov 26, 2024

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 thresholds sooner and then you wouldn’t need to pass the temporal argument down here. Eh, this is fine.

@@ -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;
Copy link
Member

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).

Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot.contour does not support date values
2 participants