Skip to content
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

allow rank to take a comparator #237

Merged
merged 2 commits into from Oct 2, 2021
Merged

allow rank to take a comparator #237

merged 2 commits into from Oct 2, 2021

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 2, 2021

No description provided.

@Fil
Copy link
Member

Fil commented Oct 2, 2021

I don't know if it helps, but the comparator is available in https://observablehq.com/@d3/rank-dev, as well as multiple accessors.

@mbostock mbostock marked this pull request as ready for review October 2, 2021 06:01
@mbostock mbostock requested a review from Fil October 2, 2021 06:02
Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works :). And I love the simplicity of the example.

Before I saw the new commits I pushed https://github.com/d3/d3-array/tree/fil/rank-comparator.

It adds:

  • accepts multiple accessors too, on par with d3.sort.
  • a few unit tests
  • moves the documentation of d3.rank below #sort.

let me know if you think any of this should be included.

@mbostock
Copy link
Member Author

mbostock commented Oct 2, 2021

Thanks! I think it’s nice for d3.rank to be next to d3.quantile since it feels more closely related than d3.sort.

I don’t think I want to commit to multiple accessors at this point. We don’t do that for d3.bisector, for example, and at the least it’s always possible to express as an equivalent comparator now that comparators are supported.

Also, and I’m not sure whether to file a separate issue for this, but using function.length to differentiate an accessor from a comparator feels brittle in retrospect. For example, you’d hope that d3.sort(values, Math.random) would interpret Math.random as an accessor, but since Math.random’s function.length is zero it gets interpreted as a comparator. This confusion bubbles up to Plot e.g. observablehq/plot#564. So, I’m tempted to assume an accessor if function.length !== 2 instead of if function.length === 1; but that change isn’t really backwards-compatible, so we’re a bit stuck.

@mbostock mbostock merged commit 81f2c7c into main Oct 2, 2021
@mbostock mbostock deleted the mbostock/rank-comparator branch October 2, 2021 15:26
@Fil
Copy link
Member

Fil commented Oct 2, 2021

I think it's fair to test function.length < 2 rather than function.length === 1 — is there a use case where a comparator would work with 0 argument? The only one I can think of is if someone did sort on a constant function (to not sort), but then it would also not sort.

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.

None yet

2 participants