split map and typedMap #1305
split map and typedMap #1305
Conversation
mbostock
left a comment
mbostock
left a comment
There was a problem hiding this comment.
Yes I think based on our usage we should remove the type argument to typedMap entirely and always use float64. We could call it floatMap or numberMap if you prefer.
|
No, I don’t think we should retire it if it’s part of the public API. |
|
I want to think about this one a little more and push a few commits. In a case like this floatMap(X2, (x2, i) => Math.abs(x2 - X1[i]))we are doing too much work because we are coercing the result of the accessor even though we already know that it can only return a number. |
|
In 0e74a1e I reverted the addition of floatMap, and instead isolated the implicit number coercion to valueof. This change avoids unnecessary coercion when we call map and we know that the provided accessor is already number-safe. (Well, I also changed arrayify, but I actually think that’s a bit of mistake since this implicit coercion is only needed when arrayify is used to create a typed array, and that only happens via valueof. So I’d like to follow this up with another slight change.) |
| import {Channels} from "../channel.js"; | ||
| import {create} from "../context.js"; | ||
| import {labelof, identity, arrayify} from "../options.js"; | ||
| import {labelof, identity, arrayify, map} from "../options.js"; |
There was a problem hiding this comment.
Oops, this fixes the calls here: 😳
Lines 125 to 126 in 1186b5b
| 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, Array); | ||
| if (typeof thresholds !== "number") return arrayify(thresholds); |
There was a problem hiding this comment.
Since I removed the type argument to arrayify, I adopted map(T, …) above instead of T.map(…).
| // all grids and use this to compute thresholds. | ||
| let T = thresholds; | ||
| if (!isTypedArray(T)) { | ||
| if (!(T instanceof TypedArray)) { |
There was a problem hiding this comment.
(Taking your advice on using instanceof directly… though I think the performance will be the same and maybe it’s slightly more readable to have the helper. I’m happy either way.)
| import {curveAuto, PathCurve} from "../curve.js"; | ||
| import {Mark} from "../mark.js"; | ||
| import {coerceNumbers} from "../scales.js"; | ||
| import {coerceNumbers} from "../options.js"; |
There was a problem hiding this comment.
I moved the coercion routines up to options.js since coerceNumbers is now needed for valueof. I wanted to keep coerceDates together, but I left coerceSymbols where it was since that has more dependencies…
| ? arrayify(data) // preserve undefined type | ||
| : data instanceof type | ||
| ? data | ||
| : type.prototype instanceof TypedArray && !(data instanceof TypedArray) |
There was a problem hiding this comment.
Bonus: if we already have a typed array, we can skip the coercion (even if we have to change the array type)!
|
Overall it's a good cleanup. I'm still a bit nervous with all the changes, but hopefully the tests have good coverage. |
Addressing this comment.
One tiny additional thing we could do is make Float64Array the default, since we only ever use this specific type when calling typedMap. But, I don't know, it feels a bit noisy.