Network: force array order when sorting hierarchical levels #3576

wimrijnders · 2017-10-15T07:48:04Z

Fixes #3403.
Fixes #3542.

If coordinates are not available to sort within a hierarchical level, sort to array order instead.

The previous fix on this issue was not good enough to circumvent this quirk in the chromium sorting. This should bury it.

Fixes #340r34. If coordinates are not available to sort within a hierarchical level, sort to array order instead. The previous fix on this issue was not good enough to circumvent this quirk in the chromium sorting. This should bury it.

macleodbroad-wf · 2017-10-15T13:57:15Z

lib/network/modules/components/DirectionStrategy.js

-      // Test on 'undefined' takes care of divergent behaviour in chrome
-      if (a.y === undefined || b.y === undefined) return 0;   // THIS HAPPENS
+      if (a.y === undefined || b.y === undefined) {
+        return idOrder[a.id] - idOrder[b.id];


Could you use return nodeArray.indexOf(a) - nodeArray.indexOf(b) instead of creating idOrder?

That was my first attempt. It doesn't work because the array get changed inline as the sorting runs.

So the indexing may change on every call to the custom sort() - and it does on chrome, even if there's no reason to change it. That's the quirk this issue tries to work around.

Basically, the fix is to register the initial sorting order and retain that by default.

Why are the x or y values ever undefined?

They come from the options. They are user-defined starting positions, not required. They can be undefined for that reason.

I am somewhat uncomfortable with the idea of having to iterate this array twice in order to sort it.

If the user specifies an undefined position, can we assign it a position, so that by the time it reaches this sort code, it'll never be undefined?

What about just comparing a.id against b.id in the case that x or y is undefined?

wimrijnders · 2017-10-15T15:04:31Z

I see no way of escaping it. But it's not too bad. First iteration is linear, second O(n*log(n)) if the implementers did their work right. Feel free to make a suggestion for improvement. An option could be to make the collection of id order chrome only.

wimrijnders · 2017-10-15T15:07:44Z

Copying the array with Object.assign would work as well. Would that be faster?

mbroad · 2017-10-15T15:25:57Z

Related issue https://bugs.chromium.org/p/v8/issues/detail?id=90 for reference.

mbroad · 2017-10-15T15:37:19Z

Additional discussion: https://stackoverflow.com/questions/1427608/fast-stable-sorting-algorithm-implementation-in-javascript

wimrijnders · 2017-10-15T15:54:59Z

This very sort routine is part of that determination of positions. It precedes the actual assignments. You should keep in mind that initial positions *may* be assigned. Networks with some nodes having predefined positions and some not are possible.

wimrijnders · 2017-10-15T15:56:46Z

Id's are also user-definable. You could use strings with your favorite singer names for them (real world example). The id's imply nothing about position.

macleodbroad-wf · 2017-10-15T16:02:10Z

It's true that the ids are not guaranteed to sort in an expected order, but the example shown uses sortable ids, and I think that is what most people use (ie, monotonically increasing integers). So we should optimize for that case.

Optionally we could provide a way for users to use stable versions of these sorting strategies, that take the performance hit, while others, who do not have the same problem will not take any additional performance hit.

wimrijnders · 2017-10-15T16:15:36Z

I think that is what most people use (ie, monotonically increasing integers). So we should optimize for that case.

No, sorry, disagree with this. Users insert what they damn well please. Integers is just a very special case.

Optionally we could provide a way for users to use stable versions of these sorting strategies

Hokay, this could be workable. So adding an option to be able to select a stable sort.
Another option is to find a really fast js implementation and use that instead. Perhaps wishful thinking? I'll google around now.

macleodbroad-wf · 2017-10-15T16:20:18Z

Unfortunately we do not have the actual data to back up how users id their nodes in the wild. However, if it's going to be arbitrarily sorted anyways, making it sort consistently for integer-based ids makes sense to me - especially given the issue reporter's example.

From what I've read, timsort is the best alternative... However, even it has 38% slow-down. I agree with the decision of the Chrome team to provide a fast unstable sort as the default.

edit: Also to note, the vast majority of our network examples, themselves also use monotonically increasing integers as ids.

wimrijnders · 2017-10-15T16:27:13Z

Unfortunately we do not have the actual data to back up how users id their nodes in the wild.

Well, I've got a pretty clear idea from all the examples I've collected from the issues. From my point of view, integer id's are the exception. You want me to take samples from the code of 76 issues I have at hand?

I just added some links to previous comment; perhaps they are of use. First link appears to be more efficient that what I have thought up. The other two just look interesting.

macleodbroad-wf · 2017-10-15T16:32:01Z

I'd like to throw https://www.npmjs.com/package/timsort into the running (benchmarks included on the page)

wimrijnders · 2017-10-15T16:40:01Z

Yeah. That looks good.

Edit: Very good indeed. Not only stable, but faster than default sort, at least in node.js. That's two reasons for using it.

If this beats chrome's sort, then it's a no-brainer.

wimrijnders · 2017-10-15T17:01:31Z

Ah nice, the benchmark is on github. We will test this in chrome.

wimrijnders · 2017-10-16T05:23:25Z

Here are the results of the timsort benchmark when run on chromium. Compare these with the values on the timsort page.

ArrayType                     Length TimSort   array.sort  Speedup
randomInt                     10     812       1009        1.2434146066211673
randomInt                     100    8540      4717        0.5524051126935311
randomInt                     1000   118381    59342       0.5012829312070081
randomInt                     10000  1457349   4293133     2.9458492011760473
descendingInt                 10     562       1463        2.6001051283441825
descendingInt                 100    990       10056       10.157394158751922
descendingInt                 1000   3126      166638      53.30307876796892
descendingInt                 10000  27500     2577566     93.72969696891316
ascendingInt                  10     741       1102        1.48623291403758
ascendingInt                  100    1130      10230       9.047387427267074
ascendingInt                  1000   3758      168373      44.79514466122507
ascendingInt                  10000  28766     2407033     83.67439165623504
ascending3RandomExchangesInt  10     824       1173        1.4232006910845008
ascending3RandomExchangesInt  100    1922      10480       5.452377855918806
ascending3RandomExchangesInt  1000   5891      170291      28.905792488695706
ascending3RandomExchangesInt  10000  44366     2431466     54.8039068368716
ascending10RandomEndInt       10     968       1459        1.5077770604740948
ascending10RandomEndInt       100    2504      12416       4.958007520053549
ascending10RandomEndInt       1000   7671      166860      21.751344304845748
ascending10RandomEndInt       10000  53616     2326699     43.3950885916376
allEqualInt                   10     749       1075        1.4357880569226662
allEqualInt                   100    1123      2414        2.149150782468075
allEqualInt                   1000   3933      15872       4.034953924340162
allEqualInt                   10000  30650     148133      4.833061446428325
manyDuplicateInt              10     931       1426        1.5313993235105956
manyDuplicateInt              100    8209      13010       1.5847781071129106
manyDuplicateInt              1000   109587    195935      1.7879320177946991
manyDuplicateInt              10000  1387483   2700683     1.9464618193616947
someDuplicateInt              10     957       1407        1.469996605358849
someDuplicateInt              100    8370      12928       1.5445712123971784
someDuplicateInt              1000   109951    193346      1.758472504859978
someDuplicateInt              10000  1400416   2647250     1.8903302588522226

Not too shabby. Most speedups are less than with node.js, but I suspect that's the google developers are doing a better job at sorting. On the whole, it's a gain.
Edit: Actually, node.js also uses V8, same as chrome, so that the values should be comparable. Perhaps it's my imagination that chrome performs better.

wimrijnders · 2017-10-16T05:26:35Z

While I'm at it, here are the values for firefox.

ArrayType                     Length TimSort   array.sort  Speedup
randomInt                     10     425       845         1.9875580461234132
randomInt                     100    7160      7439        1.0389516682764817
randomInt                     1000   93442     96121       1.028667362281608
randomInt                     10000  1196216   1313066     1.0976829727055877
descendingInt                 10     245       674         2.7466001966968503
descendingInt                 100    1644      4151        2.524041141007176
descendingInt                 1000   12180     47897       3.932471264366038
descendingInt                 10000  117516    647433      5.509289462489311
ascendingInt                  10     132       365         2.759715535419072
ascendingInt                  100    686       1760        2.56217396579009
ascendingInt                  1000   3821      25473       6.666339548596877
ascendingInt                  10000  30333     248116      8.179670329679569
ascending3RandomExchangesInt  10     264       664         2.51461379516853
ascending3RandomExchangesInt  100    1733      3832        2.2113563152102715
ascending3RandomExchangesInt  1000   6212      44614       7.181488933601303
ascending3RandomExchangesInt  10000  42066     521183      12.38946117273561
ascending10RandomEndInt       10     470       849         1.8044713001603132
ascending10RandomEndInt       100    2081      4480        2.1523801897787744
ascending10RandomEndInt       1000   6983      43262       6.194737784121725
ascending10RandomEndInt       10000  44433     589250      13.261440360109248
allEqualInt                   10     140       439         3.1253664169550337
allEqualInt                   100    797       1849        2.3199540085729513
allEqualInt                   1000   4017      23721       5.904480398251089
allEqualInt                   10000  30550     250083      8.186033824314205
manyDuplicateInt              10     572       912         1.593256611271726
manyDuplicateInt              100    10829     7712        0.7121805575863754
manyDuplicateInt              1000   136344    99524       0.7299497598005458
manyDuplicateInt              10000  1404733   1292216     0.9199017607137718
someDuplicateInt              10     561       841         1.5001633817054976
someDuplicateInt              100    10862     7642        0.703608801055636
someDuplicateInt              1000   137305    97024       0.7066385055165223
someDuplicateInt              10000  1443833   1368566     0.9478702527993484

Interestingly, firefox does a better job at sorting than chrome.

wimrijnders · 2017-10-16T07:22:00Z

It looks like timsort is abandonware, though. No PR's or issues processed since October '16. Not a good sign.

mbroad · 2017-10-16T22:32:46Z

That's true, but it is a fairly straightforward implementation of a well-known algorithm.

wimrijnders · 2017-10-17T03:21:24Z

OK made the switch. Interestingly, there was only one other location within Network where sorting was used.

mbroad

Just a couple nits, otherwise looks good.

mbroad · 2017-10-17T12:00:22Z

lib/network/modules/components/DirectionStrategy.js

+   * even if the custom sort function returns 0.
+   *
+   * For this reason, an external sort implementation is used,
+   * which has the added benefit of being faster of the standard


"... of being faster than the ..."

mbroad · 2017-10-17T12:02:10Z

lib/network/modules/LayoutEngine.js

@@ -30,6 +29,8 @@
 * The layout is a way to arrange the nodes in the view; this can be done
 * on non-hierarchical networks as well. The converse is also possible.
 */
+'use strict';


Why move this

No special reason, other than to keep the javascript code together. You'd rather have it on top?

Fine as is, just curious

…3576) * Network: force array order when sorting hierarchical levels Fixes #340r34. If coordinates are not available to sort within a hierarchical level, sort to array order instead. The previous fix on this issue was not good enough to circumvent this quirk in the chromium sorting. This should bury it. * Added TimSort for sorting in DirectionStrategy * Added TimSort to LayoutEngine * Fixed typo

wimrijnders added Confirmed Bug Network labels Oct 15, 2017

macleodbroad-wf reviewed Oct 15, 2017

View reviewed changes

wimrijnders mentioned this pull request Oct 16, 2017

How can I get a pull request in? mziccard/node-timsort#15

Open

wimrijnders added 2 commits October 17, 2017 05:09

Added TimSort for sorting in DirectionStrategy

3bbb21b

Added TimSort to LayoutEngine

795d62c

mbroad suggested changes Oct 17, 2017

View reviewed changes

Fixed typo

6b5c589

mbroad approved these changes Oct 17, 2017

View reviewed changes

wimrijnders mentioned this pull request Oct 18, 2017

The node display order varies from one browser to another #3542

Closed

Merge branch 'develop' into issue3404

b85b50d

yotamberk approved these changes Oct 20, 2017

View reviewed changes

yotamberk merged commit 88f0a6a into almende:develop Oct 20, 2017

wimrijnders deleted the issue3404 branch October 21, 2017 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network: force array order when sorting hierarchical levels #3576

Network: force array order when sorting hierarchical levels #3576

wimrijnders commented Oct 15, 2017 •

edited

Loading

macleodbroad-wf Oct 15, 2017 •

edited

Loading

wimrijnders Oct 15, 2017

macleodbroad-wf Oct 15, 2017

wimrijnders Oct 15, 2017

macleodbroad-wf Oct 15, 2017

mbroad Oct 15, 2017

macleodbroad-wf Oct 15, 2017

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

mbroad commented Oct 15, 2017

mbroad commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

macleodbroad-wf commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 •

edited

Loading

macleodbroad-wf commented Oct 15, 2017 •

edited

Loading

wimrijnders commented Oct 15, 2017

macleodbroad-wf commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 •

edited

Loading

wimrijnders commented Oct 15, 2017

wimrijnders commented Oct 16, 2017 •

edited

Loading

wimrijnders commented Oct 16, 2017 •

edited

Loading

wimrijnders commented Oct 16, 2017

mbroad commented Oct 16, 2017

wimrijnders commented Oct 17, 2017

mbroad left a comment

mbroad Oct 17, 2017

mbroad Oct 17, 2017

wimrijnders Oct 17, 2017

mbroad Oct 17, 2017

Network: force array order when sorting hierarchical levels #3576

Network: force array order when sorting hierarchical levels #3576

Conversation

wimrijnders commented Oct 15, 2017 • edited Loading

macleodbroad-wf Oct 15, 2017 • 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

wimrijnders commented Oct 15, 2017 via email • edited Loading

wimrijnders commented Oct 15, 2017 via email • edited Loading

mbroad commented Oct 15, 2017

mbroad commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 via email • edited Loading

wimrijnders commented Oct 15, 2017 via email • edited Loading

macleodbroad-wf commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 • edited Loading

macleodbroad-wf commented Oct 15, 2017 • edited Loading

wimrijnders commented Oct 15, 2017

macleodbroad-wf commented Oct 15, 2017

wimrijnders commented Oct 15, 2017 • edited Loading

wimrijnders commented Oct 15, 2017

wimrijnders commented Oct 16, 2017 • edited Loading

wimrijnders commented Oct 16, 2017 • edited Loading

wimrijnders commented Oct 16, 2017

mbroad commented Oct 16, 2017

wimrijnders commented Oct 17, 2017

mbroad 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

wimrijnders commented Oct 15, 2017 •

edited

Loading

macleodbroad-wf Oct 15, 2017 •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 via email •

edited

Loading

wimrijnders commented Oct 15, 2017 •

edited

Loading

macleodbroad-wf commented Oct 15, 2017 •

edited

Loading

wimrijnders commented Oct 15, 2017 •

edited

Loading

wimrijnders commented Oct 16, 2017 •

edited

Loading

wimrijnders commented Oct 16, 2017 •

edited

Loading