ascendingDefined

[mbostock]

This introduces two new comparators, d3.ascendingDefined and d3.descendingDefined, which are similar to d3.ascending and d3.descending respectively but which consider undefined, null, and non-orderable values (where !(x >= x)) greater than all other values.

d3.ascendingDefined replaces d3.ascending as the default comparator for d3.sort, d3.bisector, d3.groupSort, and d3.quickselect. However, d3.ascending remains the default for d3.greatest, d3.greatestIndex, d3.least, d3.leastIndex, where we always want to ignore undefined, null, and non-orderable values.

Note that per ECMAScript specification §23.1.3.27.1, array.sort does not invoke the specified comparator for undefined values, which are always put at the end.

Fixes #217.

[mbostock]

A moderately spicy potential addition to this PR: rename d3.ascending to d3.ascendingStrict, and d3.ascendingDefined to d3.ascending. That way you can array.sort(d3.ascending) and it does what you’d expect, and you only need d3.ascendingStrict for strict cases such as d3.greatest. But this would be going back on #210, in effect.

[Fil]

I think I'd prefer the spicy option, to keep d3.ascending as the go-to comparator.

[mbostock]

Hmm, I’m torn. Semantically, I think the “go-to” would be the strict version: given undefined input (and treating null and non-orderable values as equivalent to undefined), it feels wrong for d3.ascending and d3.descending to return a defined order. A defined order for undefined values only feels necessary in the context of sorting, or producing a total order of a set of values as when bisecting, not for an individual comparison.

Also, even with d3.ascendingDefined, array.sort doesn’t do exactly what we want: it puts strictly undefined values at the very end, without invoking the comparator. It does not put all null, undefined, and non-orderable values at the end, while respecting their relative order.

And given that ECMAScript bakes the special undefined behavior into array.sort rather than leaving it up to the comparator, I’m wondering if the same should be true of D3 methods, rather than having the comparator define the order of all values (including undefined values).

I’m feeling indecisive today. 🙂

[curran]

Think of someone new to D3 who wants to sort.

Should they have to type d3.ascending or d3.ascendingDefined?

This is an argument for the spicy option.

[Fil]

Documented in #224

[mbostock]

I have merged #224 and made it so that ascendingDefined is no longer exported. This should avoid confusion about which comparator to use.

That said, I think it might be better if sort, bisect, and quickselect used the comparator to determine whether a value is orderable, i.e., if compare(x, x) !== 0. This would eliminate the need for ascendingDefined entirely, and if you implement your own comparator that returns undefined (e.g., (a, b) => a.foo - b.foo where one of a.foo or b.foo is undefined), then you’ll get the correct behavior. However, doing this for sort will probably require using a symbol to replace undefined in the array so that we can avoid ECMAScript’s treatment of undefined.

At any rate, because we’re not exposing ascendingDefined, we can fix #217 now without tying our hands for the future.

[mbostock]

Another option for sort is that we could do it in two passes: pull all the non-comparable values out of the array (retaining their original order), sort the array using the comparator, then add the non-comparable values back to the end of the sorted array.

[mbostock]

Any objection to landing this, @Fil?

[Fil]

Lgtm!

[mbostock]

On second thought, I want to think about this some more: it’s still the case here that sort(…, ascending) and sort(…, descending) will do the “wrong” thing (i.e., nondeterministic ordering) for nonorderable values rather than putting the nonorderable values at the end. And similarly if you using ascending or descending with bisector, it’ll go poorly. I want to see if it’s possible for these functions to work correctly with any comparator; then we can eliminate ascendingDefined.

[mbostock]

Superseded by #227.