handle descending domain for quantize scales #830
Conversation
Fil
commented
Mar 25, 2022
Fil
commented
src/legends/ramp.js
Outdated
| @@ -93,7 +93,8 @@ export function legendRamp(color, { | |||
|
|
|||
| // Threshold | |||
| else if (type === "threshold") { | |||
| const thresholds = domain; | |||
| const thresholds = domain.slice(); | |||
| if (color.descending) thresholds.reverse(); | |||
There was a problem hiding this comment.
It doesn’t appear that color.descending is ever set?
src/scales/quantitative.js
Outdated
| @@ -151,8 +151,10 @@ export function ScaleQuantize(key, channels, { | |||
| range, | |||
| reverse | |||
| }) { | |||
| domain = extent(domain); // TODO preserve descending domains? | |||
| const descending = order(domain) < 0; // preserve descending domain | |||
There was a problem hiding this comment.
I’m not sure we can assume that domain is an array here. I believe it’s allowed to be an iterable, so we’d need to arrayify it first. Also, would it be simpler to flip reverse here if the domain is decending, rather than reversing the thresholds later? Then we won’t need to support descending thresholds for reversed scales generally.
if (order(domain) < 0) reverse = !reverse; // preserve descending domainThere was a problem hiding this comment.
To answer my own question here, flipping reverse would produce a different result: it flips the range of the scale rather than the domain. So, you’d get this:
Instead of this:
The behavior of the scale is the same, but the appearance of the legend is different. If we want to be consistent with descending domains on continuous scales, then we want to flip the domain rather than the range.
src/scales/quantitative.js
Outdated
| @@ -168,9 +170,16 @@ export function ScaleThreshold(key, channels, { | |||
| range = interpolate !== undefined ? quantize(interpolate, domain.length + 1) : registry.get(key) === color ? ordinalRange(scheme, domain.length + 1) : undefined, | |||
| reverse | |||
| }) { | |||
| const descending = domain[0] > domain[1]; | |||
There was a problem hiding this comment.
We’d need to use order(domain) here again. But as I hinted at in the other comment, I don’t think we need to add support for descending domains for the threshold scales; we only need to do it for quantize scales. Threshold scales require you to specify explicit thresholds rather than an extent, so it seems reasonable to require that those thresholds are in ascending order.
Also, it looks like ScaleThreshold doesn’t correctly handle when the domain is specified as an iterable rather than an array (because it references domain.length), and it probably should.
src/scales/quantitative.js
Outdated
| @@ -168,9 +170,16 @@ export function ScaleThreshold(key, channels, { | |||
| range = interpolate !== undefined ? quantize(interpolate, domain.length + 1) : registry.get(key) === color ? ordinalRange(scheme, domain.length + 1) : undefined, | |||
| reverse | |||
| }) { | |||
| const descending = domain[0] > domain[1]; | |||
| if (descending) domain.reverse(); | |||
There was a problem hiding this comment.
This would mutate the caller’s domain! Don’t mutate inputs! 😬
* quantize scale * tests * document quantize scales * handle descending domain for quantize scales (#830) * reverse and descending * tests for quantize scales * remove dead code * update test snapshot * handle non-array domain Co-authored-by: Mike Bostock <[email protected]> * Update README * exact thresholds when range is specified * rename quantiles option to n * Update README Co-authored-by: Philippe Rivière <[email protected]>
