feat: order ordinal values by sum #814
Conversation
nickofthyme
added
enhancement
There was a problem hiding this comment.
Works great, added some notes, the request part is the removal of the word bucket and answering if making the API more general would make sense, to avoid a future breaking change when some other agg eg. count or avg is added, and serve the asc order which is equally important to desc.
| BasicSeriesSpec, | ||
| 'id' | 'data' | 'xAccessor' | 'yAccessors' | 'y0Accessors' | 'splitSeriesAccessors' | 'markSizeAccessor' | ||
| >, | ||
| xValueSums: Map<string | number, number>, |
There was a problem hiding this comment.
Is it important to bake in the fact that we're sorting by sum at the moment? It could be different in the future, e.g. sort by count, min, max, average, or even, something totally unrelated to what's currently visualized
There was a problem hiding this comment.
Yeah I agree this could be more extensible to use count, min, max, average or a custom metric. However, I think this can be addressed in #795 to provide a more robust api for such sorting.
This feature is another final thing needed for the vislib replacement. I was debating whether to add it to the SettingsSpec or just use a blind prop. What do you think?
The only thing I think that might be worth it, is to make orderOrdinalBucketsBySum into asc | desc instead of a boolean, but beyond that is a little out of scope.
There was a problem hiding this comment.
I think this can be addressed in #795 to provide a more robust api
So that will be a breaking change, isn't it simpler to avoid it? I wasn't suggesting implementing count, min, max etc. right now (ie. "sum" is enough, and can even be the default), just trying to avoid names in the API that we know will be outdated in no time. So mostly, a naming thing and maybe, if feasible, a type signature that permits a string for an agg spec and a direction spec as optionals (or something similar).
whether to add it to the SettingsSpec or just use a blind prop
I don't have as much info as you do; the feature, albeit triggered by vislib, isn't vislib specific, so if we go for a somewhat stable API (see earlier point) I'd put it in SettingsSpec, if it's super temporary, then a blind one is OK I guess. Depends in part on what you think the consumer will be happy with
There was a problem hiding this comment.
Thanks Nick for the changes!
The only item remaining is that, if I understand, orderOrdinalBinsBySum would soon get obsoleted with the introduction of alternative ordering aggs (count, min, max, avg etc.), we could remove Sum from the name and pick something like { binAgg = 'sum', direction = 'desc' } as value. But you likely have more info on the tradeoff and need for expediency.
monfera
changed the title
|
jenkins retest this please |
|
🎉 This PR is included in version 21.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [21.2.0](elastic/elastic-charts@v21.1.2...v21.2.0) (2020-09-14) ### Features * blind sorting option for vislib ([opensearch-project#813](elastic/elastic-charts#813)) ([79d0e4f](elastic/elastic-charts@79d0e4f)) * order ordinal values by sum ([opensearch-project#814](elastic/elastic-charts#814)) ([45ea0c6](elastic/elastic-charts@45ea0c6)) * **series:** add simple mark formatter ([opensearch-project#775](elastic/elastic-charts#775)) ([75bd8d5](elastic/elastic-charts@75bd8d5))
Summary
Sort ordinal labels by sum of all bin values. The
orderOrdinalBinsBySumoption is added to theSettingsspec to enable this functionality. Allows ascending and descending order.These changes only affects
Ordinalscales.Screenshots
Checklist
src/index.ts(and stories only import from../srcexcept for test data & storybook)