sort categorical Cartesian axes by value #3864
Conversation
|
For reasons unknown, running the tests on CircleCI today resulted in a ridiculously low success rate 😞 |
Have you rebased off master since #3859 got merged? |
Yes I did! |
Cool. Looks like |
| }, { | ||
| "type": "bar", | ||
| "x": ["a", "b", "c"], | ||
| "y": [10, 20, 30], |
There was a problem hiding this comment.
Hmm. I don't understand why the categories show up as [b, c, a]. Shouldn't the y values for the bar trace by considered also?
There was a problem hiding this comment.
Hmm. I don't understand why the categories show up as
[b, c, a]. Shouldn't theyvalues for the bar trace by considered also?
At the moment, not if they are on a different axis: only the traces associated with the axis to be sorted are accounted for. In this mock, the bar trace is on different axes namely (x|y)axis2. It ends up inheriting the order of categories of xaxis because it matches it but that's all. I thought this behavior would be useful.
We could account for the values of all traces across all matching axes IF we also support a way to specify traces to ignore as suggested by @nicolaskruchten (see #3606 (comment)) so that we can reproduce the current baseline.
There was a problem hiding this comment.
not if they are on a different axis: only the traces associated with the axis to be sorted are accounted for. In this mock, the
bartrace is on different axes namely(x|y)axis2. It ends inheriting the order of categories ofxaxisbecause itmatchesit but that's all. I thought this behavior would be useful.
Thanks for the explanation. That makes perfect sense 👍
|
Nicely done @antoinerg ! Test coverage looks impressive. I would be ok leaving What I would like to see though: the sorting logic in https://github.com/plotly/plotly.js/blob/master/src/plots/cartesian/category_order_defaults.js getting moved to |
I ended up adding support for |
src/plots/plots.js
Outdated
| 'sum': function(values) {return Lib.aggNums(function(a, b) { return a + b;}, null, values);}, | ||
| 'value': function(values) {return Lib.aggNums(function(a, b) { return a + b;}, null, values);}, | ||
| 'mean': function(values) {return Lib.mean(values);}, | ||
| 'median': function(values) {values.sort(); var mid = Math.round((values.length - 1) / 2); return values[mid];} |
There was a problem hiding this comment.
I'm surprised our linter didn't complain here. Would you mind rewriting this as
'median': function(values) {
values.sort();
var mid = Math.round((values.length - 1) / 2);
return values[mid];
}There was a problem hiding this comment.
There's probably an eslint rule for this 🔬
|
As discussed in the original issue #3606 (comment), in the future, we will need to add new attributes to support different ways of sorting categories. @etpinard suggested that we create a new container to group all those attributes. I think it would be a good idea. Right now, everything is stuffed into Please let me know if you see any problems with this approach and whether the current state of this PR can be merged as is. |
|
My take on #3864 (comment) : Our All in all, this PR LGTM. Well-earned 💃 |
Closes #3606 by introducing the possibility to sort by values using
categoryorder.Most of the work involved writing tests to make sure everything works properly across all traces. Note that
scatterglandsplomrequired special handling since they differ from other traces.Supported traces (:lock: down with tests of course):
Supported aggregations:
I chose aggregations (
min,maxandsum) because they are unambiguously defined regardless of the order of operation (the minimum of the traces' minimum is equal to the minimum of all values across all traces). Other operations such as "take the mean" are ambiguous: is it the mean of all values across all traces or the mean of each traces' means (at the moment we assume the former for mean/median). To be clear, we need the ability to specify how to aggregate at each step:This will probably require making a new attribute. Do you have any suggestions for this? Related #3606 (comment)
Coverage:
TODO:
indexOfcallsFUTURE WORK:
Plots.doCalcData(sort categorical Cartesian axes by value #3864 (comment))cc @nicolaskruchten @plotly/plotly_js