-
Notifications
You must be signed in to change notification settings - Fork 62
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
null, undefined, NaN and -Infinity create holes #61
Conversation
…es when computing the thresholds. closes #49
src/contours.js
Outdated
v1 = low(values[yt * dx + xt]); | ||
if (x > 0 && x < dx && xt === x) { | ||
v0 = low(values[yt * dx + xt - 1]); | ||
if (v0 == null || isNaN(v0) || !isFinite(v0)) v0 = -1e52; |
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.
This…
- is redundant with
low(…)
- treats positive infinity as negative infinity
- uses a magic number when we can handle infinity exactly
The passed-in value
should be coerced like this (outside this function, probably in contour
as I mentioned in the other comment):
value = value === null ? NaN : +value;
if (isNaN(value)) throw new Error(`invalid value: ${value}`);
Values we read from values
should be coerced like this:
v0 = v0 === null ? NaN : +v0;
v1 = v1 === null ? NaN : +v1;
Given that, maybe something like this?
const a = value - v0;
const b = v1 - v0;
const d = isFinite(a) && isFinite(b) ? a / b : Math.sign(a) / Math.sign(b);
point[0] = isNaN(d) ? x : x + d - 0.5;
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.
Per above comments. Thanks for taking on this work!
Co-authored-by: Mike Bostock <mbostock@gmail.com>
New version following your review. Thanks! |
Thank you! I don't think this needs an addition to README, it's essentially an extension of the "applicability" (or one could say, a bug fix). |
Also, ignore infinite values when computing the thresholds.
Demo: https://observablehq.com/@d3/d3-contours-treats-nulls-as-holes-61
closes #49