Custom Sort Order #3423

sirahd · 2018-02-27T07:07:45Z

Please:

Make your pull request atomic, fixing one issue at a time unless there are many relevant issues that cannot be decoupled.
Provide a test case & update the documentation under site/docs/
Make lint and test pass. (Run yarn lint and yarn test. If your change affects Vega outputs of some examples, please run yarn build:example EXAMPLE_NAME to re-compile a specific example or yarn build:examples to re-compile all examples.)
Make sure you have rebased onto the master branch.
Provide a concise title so that we can just copy it to our release note.
- Use imperative mood and present tense.
- Mention relevant issues. (e.g., #1)

Pro-Tip: https://medium.com/@greenberg/writing-pull-requests-your-coworkers-might-enjoy-reading-9d0307e93da3 is a nice article about writing a nice pull request.

feature request (Custom sort order #2915)

kanitw · 2018-02-27T15:46:06Z

Thanks. Before we review, exclude SVG changes first? If the vega files don't change, don't include SVG changes.

sirahd · 2018-02-27T23:20:04Z

@kanitw Done!

kanitw

Great progress! Still some comments to be addressed. Please also add unit tests.

We're getting close!

kanitw · 2018-03-04T01:32:22Z

_config.yml

@@ -49,7 +49,7 @@ kramdown:
  syntax_highlighter: rouge
  auto_id_stripping: true

-plugins:
+plugins_dir:


Why is this related to this PR?

Let's put this in a separate PR.

kanitw · 2018-03-04T01:38:51Z

site/docs/encoding/sort.md

@@ -55,14 +55,22 @@ If the channel has a discrete scale (`band`, `point` or `ordinal`), the field's

 3) Unsorted – `null` – The field is not sorted. This is equivalent to specifying `sort: false` in [Vega's scales](https://vega.github.io/vega/docs/scales/#sort).

-4) Specify custom order by providing custom `scale`'s [`domain`](scale.html#domain).  (In this case, you don't need to use `sort` property.)
+4) Specify custom order by:
+  1) providing `sort` with an array that lists valid input domains


This is quite misleading.

Using scale domain literally lists "(all) valid input domains". But the whole point of having sort supports an array is to overcome this limitation and instead lists "preferred order of values".

Thus, instead of having sub 1) and 2) here. I would rather:

make it focus on sort array

explain what happen if data field contains unlisted values? + add an example

Add a __Note:__ why sort array is different from using scale domain / they should prefer using sort array unless they really want to set domain.

kanitw · 2018-03-04T01:40:36Z

src/fielddef.ts

@@ -168,12 +168,12 @@ export interface ScaleFieldDef<F> extends FieldDef<F> {

  /**
   * Sort order for the encoded field.
-   * Supported `sort` values include `"ascending"`, `"descending"` and `null` (no sorting).
+   * Supported `sort` values include `"ascending"`, `"descending"`, `"array of field's domains"` and `null` (no sorting).


"array of field domain" is misleading.

"array specifying preferred order of the values"?

Please also briefly explain what happens if sort doesn't include all domain values.

kanitw · 2018-03-04T01:46:51Z

src/compile/data/calculate.ts

+  public static calculateExpressionFromSortField(field: string, sortFields: string[]): string {
+    let expression = '';
+    let count = 0;
+    for (const sortField of sortFields) {


If you need the index anyway, better just use normal for loop and do sortFields[i].

for ... of is normally for when you don't care about index. (With babel, it expands to be full normal for loop.)

kanitw · 2018-03-04T02:16:36Z

src/compile/data/calculate.ts

@@ -15,6 +18,32 @@ export class CalculateNode extends DataFlowNode {
    super();
  }

+  public static makeAllFromSort(model: ModelWithField): CalculateNode[] {


The name is quite vague for its very specific purpose. ("Make all" of what? This is actually a special purposed calculate node, not a generic calculate node.)

Perhaps, makeAllForSortIndex is better?

(Also feel free to suggest better name.)

I also start wondering that this is quite a special purposed one, so I wonder if having a distinct type of node for this will help with optimization in longer term?
(But since this is already working, maybe it's waste of time to change it now -- I'm just asking.)

@domoritz -- any thoughts?

I don't think it is that special and I can't think of a special optimization that we would do. Let's leave it for now.

kanitw · 2018-03-04T02:19:05Z

examples/specs/bar_custom_sort.vl.json

+  },
+  "mark": "bar",
+  "encoding": {
+    "x": {"field": "a", "type": "ordinal", "sort": ["D", "G", "F"]},


Maybe it's better distinguish between (1) the example that show how sort order normally works when all items are included and (2) another example to explain behavior when some values is "accidentally" ignored?

The first can just have three values B, A, C.
The second can have 2-3 more (D,E,F?).
No need for 9 categories -- only make it more confusing.

kanitw · 2018-03-04T02:23:06Z

src/compile/data/calculate.ts

+      if (isScaleFieldDef(fieldDef) && isSortArray(fieldDef.sort)) {
+        const transform: CalculateTransform = {
+          calculate: CalculateNode.calculateExpressionFromSortField(fieldDef.field, fieldDef.sort),
+          as: `${fieldDef.field}_sort_index`


Should channel be a part of the name?
(e.g., ${channel}_${fieldDef.field}_sort_index.)

Imagine a troll case where I have x and y encode the same field, but use different sort array. Your current code will behave incorrectly.

kanitw · 2018-03-04T02:27:18Z

src/compile/scale/domain.ts

+  if (isSortArray(sort)) {
+    return {
+      op: 'min',
+      field: `${model.vgField(channel)}_sort_index`,


See https://github.com/vega/vega-lite/pull/3423/files#diff-3cdb83c52621809d58d32cb4e52787ffR27

kanitw · 2018-03-04T02:27:42Z

src/compile/scale/domain.ts

@@ -266,6 +266,15 @@ export function domainSort(model: UnitModel, channel: ScaleChannel, scaleType: S

  const sort = model.sort(channel);

+  // if the sort is specified with array, use the created data


use the derived sort index field

kanitw · 2018-03-04T18:09:49Z

src/compile/data/parse.ts

@@ -249,6 +249,11 @@ export function parseData(model: Model): DataComponent {
      tu.parent = head;
      head = tu;
    }
+
+    for (const calculate of CalculateNode.makeAllFromSort(model)) {


Also update visual diagram on the top.

kanitw

We're getting close! Still some changes needed though. Keep up the good work!

kanitw · 2018-03-06T17:26:41Z

site/docs/encoding/sort.md

-4) Specify custom order by:
-  1) providing `sort` with an array that lists valid input domains
-  2) providing custom `scale`'s [`domain`](scale.html#domain)
+4) Specify custom order by providing custom `scale`'s [`domain`](scale.html#domain). (In this case, you don't need to use `sort` property.)


As mentioned earlier, this shouldn't be a "suggested" way for sorting. Plus, putting unpreferred way on top of preferred way is weird.

Please just make this like a note that it can achieve similar effect but has some pitfall(s). (The sort array can be 4. and this should become just a note.)

Actually, we should also mention about sort in scale's domain documentation too.

I think we already have that in domain documentation (https://vega.github.io/vega-lite/docs/scale.html#domain)

Yes, but it didn't say what's the different between domain array and sort array yet so briefly mention about it and link to the full example in sort page would be nice.

kanitw · 2018-03-06T17:28:53Z

site/docs/encoding/sort.md


-#### Example: Sorting Ordinal Scale by Another Field
+5) Sorting by preferred order of values by providing `sort` with array specifying preferred order of the values. Unspecified values will assume their original orders in the data source, preceded by the orders in the sort array.


Sorting by preferred order of values by providing sort with array specifying preferred order of the values.

"preferred order of values" showing up twice, quite redundant.

"providing sort as an array specifying preferred order of the values." ?

kanitw · 2018-03-06T17:31:02Z

src/fielddef.ts

@@ -169,8 +169,9 @@ export interface ScaleFieldDef<F> extends FieldDef<F> {

  /**
   * Sort order for the encoded field.
-   * Supported `sort` values include `"ascending"`, `"descending"`, `"array of field's domains"` and `null` (no sorting).
+   * Supported `sort` values include `"ascending"`, `"descending"`, `"array specifying the preferred order of values"` and `null` (no sorting).


"array specifying the preferred order of values" suggests that this is an enum array, so it's a bit misleading.

"ascending", "descending", null, or an array specifying the preferred order of values

kanitw · 2018-03-06T17:31:14Z

src/fielddef.ts

   * For fields with discrete domains, `sort` can also be a [sort field definition object](https://vega.github.io/vega-lite/docs/sort.html#sort-field).
+   * For `sort` as an "array specifying the preferred order of values", the sort order will obey the values in the array, followed by any unspecified values in their original order.


Why quoting?

Also add link to the section that you provide an example for sort array.

kanitw · 2018-03-06T17:31:59Z

site/docs/encoding/sort.md


-The following example sorts x by mean of Horsepower.
+In the case that sort array contains every field value, the sort order will follow the specified values in the array.
+<div class="vl-example" data-name="bar_custom_sort_full"></div>


line break before DIV?

kanitw · 2018-03-06T17:32:02Z

site/docs/encoding/sort.md


-<div class="vl-example" data-name="histogram_sort_mean"></div>
+Sort order will precede by the specified values in the array, even though some value is ignored. Note how `D`, `E` and `F` still follows their original order, while `B`, `A` and `C` follows the order in sort array.
+<div class="vl-example" data-name="bar_custom_sort_partial"></div>


line break before DIV?

kanitw · 2018-03-06T17:33:51Z

Oh and please make tests pass. :)

kanitw · 2018-03-07T16:34:45Z

site/static/main.ts

@@ -72,7 +72,7 @@ function embedExample($target: any, spec: TopLevelExtendedSpec, actions=true, to
  });

  if (tooltip) {
-    vegaLite(view, spec);


can you fix this in a separate PR that we can undo easily

fix jekyll error and linting

create examples and fix jekyll error in base.html

undo jekyll errors

sirahd · 2018-03-18T08:30:33Z

@kanitw I just rebased. It should be good to go!

kanitw

Thanks a lot! Only have a few last commennts and this should be good to go :)

I also made a pass over the docs. :)

kanitw · 2018-03-18T17:48:13Z

examples/specs/bar_custom_sort_partial.vl.json

+  "data": {
+    "values": [
+      {"a": "A","b": 28}, {"a": "B","b": 55}, {"a": "C","b": 43},
+      {"a": "D","b": 91}, {"a": "E","b": 81}, {"a": "F","b": 53}


Actually, change this to Z, Y, X?

I believe the result would be B, A, C, Z, Y, X.
(The reason we should change is that using B, A, C, D, E, F, it won't be clear whether it's D E F because of the original order or because D is naturally before E and F.)

(Please double check to make sure I'm not crazy :p)

kanitw · 2018-03-18T17:49:43Z

src/compile/data/parse.ts

@@ -202,6 +204,10 @@ export function parseData(model: Model): DataComponent {
    }

    head = TimeUnitNode.makeFromEncoding(head, model) || head;
+
+    for (const calculate of CalculateNode.makeAllForSortIndex(head, model)) {


Do we really still need for loop here? Can't we just set head to the last item in the array?

So it seems like the constructor and assemble code for CalculateNode doesn't handle the case where it has to generate multiple nodes from one instance. I could change it so that the constructor doesn't require for loop, but that would affect the code we already have for general CalculateNode

I don't understand. Why would the following code be different from the for loop you have here?

const sortIndexNodes = CalculateNode.makeAllForSortIndex(head, model); head = sortIndexNodes[sortIndexNodes.length - 1];

Can you explain more if I overlook anything?

I think what I'm trying to do might be different here. I saw that @domoritz made some changes to the way bin and timeUnit nodes are created and assembled. Previously we had to iterate over for loop to construct nodes returned from methods like makeAll, but the new changes made it so that makeAll returns only one node with all the components inside a dictionary with field name as a key(instead of multiple nodes with separate components). So the constructor of bin and timeUnit now takes the parent and the dictionary as params.

Anyway, I will follow your suggestion since I think it doesn't make sense for CalculateNode to have the same behavior as bin and timeUnit node as I explained above.

Previously we had to iterate over for loop to construct nodes returned from methods like makeAll, but the new changes made it so that makeAll returns only one node with all the components inside a dictionary with field name as a key(instead of multiple nodes with separate components). So the constructor of bin and timeUnit now takes the parent and the dictionary as params.

I think the old loop is updating head like you do down here in the past, but it's kinda annoying because you need to update head and keep passing current head to the next one.

So we incorporate head into constructor to make the code in data/parse.ts shorten by half.

returns only one node with all the components inside a dictionary with field name as a key

I think your case is more like geopoint/ geojson transform. Which basically updating parent within the parseAll method. So I think you could follow the pattern and do

head = CalculateNode.parseAllSortIndex(head, model);

where your parseAllSortIndex returns just the final head (parent)

cc: @domoritz Please correct me if I'm wrong.

kanitw · 2018-03-18T17:53:14Z

src/compile/scale/domain.ts

+  if (isSortArray(sort)) {
+    return {
+      op: 'min',
+      field: `${channel}_${model.vgField(channel)}_sort_index`,


Here you use model.vgField(channel) but in the calculate.ts file you use ${channel}_{fieldDef.field}_sort_index.

For the most cases, this should be fine, but I wonder if there are cases where we apply aggregation function for categorical value -- in such case model.vgField(channel) would return different result from fieldDef.field.

I think we should have a helper method in calculate.ts for generating this field name (from a fieldDef?) and make both calculate.ts and the code here call the same method for safety in the future?

Should we stick to vgField or just fieldDef.field as a field name?

vgField(fieldDef) ?

kanitw · 2018-03-18T17:56:41Z

src/fielddef.ts

@@ -182,12 +182,13 @@ export interface ScaleFieldDef<F> extends FieldDef<F> {

  /**
   * Sort order for the encoded field.
-   * Supported `sort` values include `"ascending"`, `"descending"` and `null` (no sorting).
+   * Supported `sort` values include `"ascending"`, `"descending"`, `array specifying the preferred order of values` and `null` (no sorting).


The sort property can be "ascending", "descending", null, or an array specifying the preferred order of values.

(Remove back ticks for the array case too since you have currently have inside backticks is not the exact string you gonna put in the JSON.)

kanitw · 2018-03-18T17:57:44Z

site/docs/encoding/sort.md

+
+<div class="vl-example" data-name="bar_custom_sort_full"></div>
+
+Sort order will precede by the specified values in the array, even though some value is ignored. Note how `D`, `E` and `F` still follows their original order, while `B`, `A` and `C` follows the order in sort array.


Z, Y, X as suggested?

sirahd · 2018-03-21T06:40:14Z

@kanitw Hopefully this fixes the issue with for loop in node parsing. I move that iteration into how CalculateNode is constructed in the first place.

…e export function for sort index field name

kanitw

Approved, provided that you make this one last change :)

kanitw · 2018-03-25T03:02:40Z

src/compile/data/calculate.ts

+      if (isScaleFieldDef(fieldDef) && isSortArray(fieldDef.sort)) {
+        const transform: CalculateTransform = {
+          calculate: CalculateNode.calculateExpressionFromSortField(fieldDef.field, fieldDef.sort),
+          as: sortArrayIndexField(model, channel)


It's weird that you're still using model and channel given that this is already under model.forEachFieldDef.

I'd say sortArrayIndexField should just accept a fieldDef

If that's the case, how can we know what the channel the given fieldDef represents?

I think we should still leave channel as a function param

Sorry for the long reply, since this function is also used in domain.ts to look up the sort field, given that only model and channel is provided in that code, I think accepting model and channel here is reasonable, even though it seems redundant in model.forEachFieldDef.

That's fair. :)

sirahd requested review from domoritz and kanitw February 27, 2018 08:50

sirahd force-pushed the sh/customsort branch from a097b13 to 749f35c Compare February 27, 2018 23:16

kanitw requested changes Mar 4, 2018

View reviewed changes

kanitw mentioned this pull request Mar 4, 2018

Replace Transform #1761

Closed

kanitw reviewed Mar 4, 2018

View reviewed changes

sirahd force-pushed the sh/customsort branch 2 times, most recently from acc88c5 to 985dbfc Compare March 6, 2018 10:11

kanitw requested changes Mar 6, 2018

View reviewed changes

sirahd force-pushed the sh/customsort branch from 1685880 to 62ec6da Compare March 7, 2018 09:59

kanitw reviewed Mar 7, 2018

View reviewed changes

sirahd force-pushed the sh/customsort branch from 62ec6da to 4079fd9 Compare March 7, 2018 18:48

sirahd added 10 commits March 17, 2018 23:53

reuse calculate node to generate new sort field

22acca0

rename the created data source to fieldname_sort_index

c30de25

update docs to include examples

45e9cad

fix jekyll error and linting

remove comments and comment dataflow debug

d260ade

create examples and fix jekyll error in base.html

uses vega-util function for array and string

857a652

undo jekyll errors

add custom sort examples

b5ea38f

update documentation

ac4d8db

add unit tests and fix comment per @kanitw

ae2b2c1

fix documentation for style issue and clariy

c070957

update schema and fix import error

f83592b

sirahd force-pushed the sh/customsort branch from 4079fd9 to 41971aa Compare March 18, 2018 08:06

fixes rebase issue

177ead1

sirahd force-pushed the sh/customsort branch from 41971aa to 177ead1 Compare March 18, 2018 08:19

kanitw requested changes Mar 18, 2018

View reviewed changes

Update sort.md

90eabb3

kanitw mentioned this pull request Mar 18, 2018

Custom sort order #2915

Closed

remove iterating in parse and instead do for loop in each node, creat…

5efd2c6

…e export function for sort index field name

sirahd force-pushed the sh/customsort branch from 9e1bf81 to 5efd2c6 Compare March 21, 2018 06:45

update schema

6fbdde7

kanitw approved these changes Mar 25, 2018

View reviewed changes

kanitw merged commit 766e010 into master Mar 29, 2018

kanitw deleted the sh/customsort branch March 29, 2018 21:10

		@@ -266,6 +266,15 @@ export function domainSort(model: UnitModel, channel: ScaleChannel, scaleType: S

		const sort = model.sort(channel);

		// if the sort is specified with array, use the created data


		#### Example: Sorting Ordinal Scale by Another Field
		5) Sorting by preferred order of values by providing `sort` with array specifying preferred order of the values. Unspecified values will assume their original orders in the data source, preceded by the orders in the sort array.

		* For fields with discrete domains, `sort` can also be a [sort field definition object](https://vega.github.io/vega-lite/docs/sort.html#sort-field).
		* For `sort` as an "array specifying the preferred order of values", the sort order will obey the values in the array, followed by any unspecified values in their original order.


		<div class="vl-example" data-name="bar_custom_sort_full"></div>

		Sort order will precede by the specified values in the array, even though some value is ignored. Note how `D`, `E` and `F` still follows their original order, while `B`, `A` and `C` follows the order in sort array.

Custom Sort Order #3423

Custom Sort Order #3423

Conversation

sirahd commented Feb 27, 2018 • edited Loading

kanitw commented Feb 27, 2018

sirahd commented Feb 27, 2018

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirahd commented Mar 18, 2018

kanitw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirahd commented Mar 21, 2018

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirahd Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirahd commented Feb 27, 2018 •

edited

Loading

kanitw left a comment •

edited

Loading

kanitw left a comment •

edited

Loading

kanitw Mar 18, 2018 •

edited

Loading

kanitw Mar 18, 2018 •

edited

Loading

sirahd Mar 27, 2018 •

edited

Loading