Skip to content

Add groupSort. - #187

Merged
mbostock merged 2 commits into
masterfrom
mbostock/group-sort
Jan 24, 2021
Merged

mbostock merged 2 commits into
masterfrom
mbostock/group-sort

Conversation

@mbostock

@mbostock mbostock commented Jan 24, 2021

Copy link
Copy Markdown
Member

For example, to sort varieties by descending median yield:

d3.groupSort(barley, g => -d3.median(g, d => d.yield), d => d.variety)

@mbostock
mbostock requested a review from Fil January 24, 2021 00:03
@Fil

Fil commented Jan 24, 2021

Copy link
Copy Markdown
Member

Concerning groupSort, could that be made to work with nested grouping?

It might be nice to write

d3.groupSort(penguins, v => -v.length, d => d.species, d => d.island)

and the API of groups/rollups would feel more consistent.

We can craft that by hand by grouping on compounded keys:

d3.sort(
    d3.rollup(penguins, v => v.length, d => [d.island, d.species].toString()),
    ([, value]) => -value
  )
  .map(([key, value]) => key.split(/,/)) // [ ["Biscoe", "Gentoo"], … ]

(the toString hack would not be needed if d3.rollup accepted arrays as keys, as in #185 (comment))

Concerning d3.medianSort and the other shorthands, I wonder if it's not too much: they're easy to write with groupSort, and having min, max, median seems incomplete (why not the mean, length, etc). It's all just syntactic sugar so more a question of taste than anything else, but it seems a bit too sugary to me :)

@mbostock

Copy link
Copy Markdown
Member Author

I don’t think nested grouping will work, at least as is — what would the comparator compare? If the comparator is passed both the leaf groups and the nested groups, the comparator will have to do some introspection to figure out what it’s comparing. If the comparator is passed the leaf groups and flattened nested groups, then it’ll work, but it won’t be very efficient and it’ll add some complexity for a use case I’m not confident is very common.

You can do the compound key thing with d3.groupSort directly:

d3.groupSort(penguins, v => -v.length, d => JSON.stringify([d.species, d.island])).map(JSON.parse)

I feel like the medianSort et al. are common enough to want, and simple enough to support, that it’s worth the trouble. It does raise the question of why no sumSort, meanSort, countSort, though… I suppose we could always start with groupSort and then add the helpers later?

@mbostock
mbostock force-pushed the mbostock/group-sort branch from d3e4463 to e7bd9a6 Compare January 24, 2021 17:39
@mbostock

Copy link
Copy Markdown
Member Author

Pruned this PR down to just groupSort, and put the helper methods in another branch in case we want to add them later.

@mbostock mbostock changed the title Add groupSort and medianSort. Add groupSort. Jan 24, 2021
@mbostock
mbostock merged commit 81dd4aa into master Jan 24, 2021
@mbostock
mbostock deleted the mbostock/group-sort branch January 24, 2021 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants