Skip to content

mark order #108

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

Closed
wants to merge 5 commits into from
Closed

mark order #108

wants to merge 5 commits into from

Conversation

mbostock
Copy link
Member

Alternative to #106.

@mbostock mbostock requested a review from Fil January 23, 2021 19:45
@mbostock mbostock mentioned this pull request Jan 23, 2021
@mbostock mbostock force-pushed the mbostock/mark-order branch from 5f0fbfc to 587320b Compare January 23, 2021 19:47
@mbostock
Copy link
Member Author

Okay, pretty excited about this. For ascending bars:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: "x"}})

Or equivalently:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: "+x"}})

For descending bars:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: "-x"}})

For input order:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: "input"}})

For reverse input order:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: "-input"}})

I need to do some plumbing to expose the order option on all marks, but this’ll work with any mark now, not just bars.

@mbostock
Copy link
Member Author

I also considered this for input order:

Plot.barX(alphabet, {x: "frequency", y: "letter", order: {y: null}})

Let me know if you prefer that. It feels a little weird to me, and doesn’t support reverse input order, but that’s not a requirement.

@mbostock
Copy link
Member Author

Okay, I prefer the null order.

@Fil
Copy link
Contributor

Fil commented Jan 23, 2021

Shouldn't {y: "-x"} be {y: "x", reverse: true}? We're treading on DSL territory. (Other than that I love the genericity of this PR.)

@mbostock
Copy link
Member Author

Yeah, I thought about that, but you can actually specify multiple orders simultaneously, so the reverse option would be ambiguous. For example, if you have an ordinal color scheme, you might say order: {y: "x", color: "x"} to order the colors by bar length, too. We could do this instead: order: {y: {channel: "x", reverse: true}} but that felt a lot more verbose than order: {y: "-x"}. 🤷

@mbostock mbostock force-pushed the mbostock/mark-order branch from 044be18 to 10b0d3e Compare January 23, 2021 20:37
@mbostock
Copy link
Member Author

It would be nice to figure out how to pass through a custom comparator function to sort the domain, though. I suppose we could always allow the more verbose syntax in the future if we want it.

@mbostock mbostock force-pushed the mbostock/mark-order branch from 10b0d3e to c657a69 Compare January 23, 2021 23:12
@mbostock
Copy link
Member Author

mbostock commented Jan 24, 2021

I am struggling with this. I added the Becker’s Barley example to the main branch which I think is reasonably representative of a real-world example of the ordering problem. I ran into issues:

  1. When sorting y based on x, this branch tacitly assumes that there is exactly one x value for each given y value. More precisely, this branch sorts based on the extreme x value for each given y. In the Becker’s Barley example, we want to sort by the median value instead. We could make the median the default behavior of the order option, but that feels like cheating.

  2. The order option can only be used to sort a scale that is associated with the current mark. So, if we want to order the y-facets (sites) by median yield, there’s no way to drive that from the dot mark because the dot mark only knows about yield and variety (and year). And the facet mark doesn’t know about yield. So we’re back to needing to specify the domain explicitly.

Perhaps we should not support an order option, and instead focus on making it easier to compute the desired sorted domain explicitly? I’ve opened a PR in d3-array for a groupSort and medianSort helper method.

d3/d3-array#187

@Fil
Copy link
Contributor

Fil commented Jan 24, 2021

The barley example (A) is very convincing in its own right, that the domains shouldn't be ordered from the marks.

But the letter frequency example (B) is at first view very convincing of the opposite, as in that case it doesn't feel right that we would do the grouping twice, with different symbols—the first time to order the bars by height, the second time to compute the bars' heights.

However, thinking with facets clarifies the situation, and shows that (B) is handy but not generalizable, because the domain order might be different on each facet. If we have 3 books in 3 languages, and order letter frequency in these books by language, then the domain order should be an "overall" order and can't be set from a facet.

So, yes to ordering the domain in the definition of the scale. "input" order can be obtained by setting domain: new Set(data.map(accessor)); and "frequency" or "bar length" order obtained by groupSort(data, v => -v.length, d => d.letter).

With the plot convention of having "field" as a shorthand notation for d => d.field, this could even be groupSort(data, "-length", "letter").

@mbostock
Copy link
Member Author

Thanks for the feedback! I feel pretty good about where this ended up; see 534cb96 and 69490ac. (The second commit is perhaps a little gratuitous, but shorter…)

For input order, note that new Set(…) is not required as this will be done automatically by d3.scaleOrdinal when assigning the domain. Though that reminds me we still need to switch d3.scaleOrdinal to use InternMap instead of using string keys.

I like the idea of d3-array supporting string accessors like Plot does. I took a cursory look at doing that, and I think it would be a fair amount of work. There were also a few ambiguities — places where I’m not sure whether a string accessor would make sense. I’ll file an issue there and we can discuss.

@mbostock mbostock closed this Jan 25, 2021
@Fil Fil mentioned this pull request Apr 28, 2021
@mbostock mbostock deleted the mbostock/mark-order branch August 19, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants