TreeViewer: Fix different trees being displayed for same dataset

OWTreeViewer was not deterministic in showing trees. This happened
because the layout was updated w.r.t the graph node edges, which were
stored in a set object, which is unordered.
I assume that a set was used because speed is important, so I've
implemented edges as an ordered dict, to keep the speed benefits as well
as an ordering.

Author: pavlin-policar

Orange/widgets/tests/datasets/same_entropy.tab

@@ -0,0 +1,7 @@
+target	d1	d2
+d	d	d
+class
+A	2	2
+A	2	2
+B	1	1
+B	1	1

Orange/widgets/visualize/owtreeviewer2d.py

@@ -1,3 +1,5 @@
+from collections import OrderedDict
+
 from AnyQt.QtGui import (
     QBrush, QPen, QColor, QPainter, QPainterPath, QTransform
 )
@@ -20,16 +22,20 @@
 
 class GraphNode:
     def __init__(self, *_, **kwargs):
-        self.edges = kwargs.get("edges", set())
+        # Implement edges as an ordered dict to get the nice speed benefits as
+        # well as adding ordering, which we need to make trees deterministic
+        self.__edges = kwargs.get("edges", OrderedDict())
 
     def graph_edges(self):
-        return self.edges
+        """Get a list of the edges that stem from the node."""
+        return self.__edges.keys()
 
     def graph_add_edge(self, edge):
-        self.edges.add(edge)
+        """Add an edge stemming from the node."""
+        self.__edges[edge] = 0
 
     def __iter__(self):
-        for edge in self.edges:
+        for edge in self.__edges.keys():
             yield edge.node2
 
     def graph_nodes(self, atype=1):
@@ -185,7 +191,7 @@ def itemChange(self, change, value):
 
     # noinspection PyCallByClass,PyTypeChecker
     def update_edge(self):
-        for edge in self.edges:
+        for edge in self.graph_edges():
             if edge.node1 is self:
                 QTimer.singleShot(0, edge.update_ends)
             elif edge.node2 is self:
@@ -274,16 +280,17 @@ def fix_pos(self, node=None, x=10, y=10):
         self.update()
 
     def _fix_pos(self, node, x, y):
+        """Fix the position of the tree stemming from the given node."""
         def brect(node):
+            """Get the bounding box of the parent rect and all its children."""
             return node.boundingRect() | node.childrenBoundingRect()
 
         if node.branches and node.isOpen:
             for n in node.branches:
-                x, ry = self._fix_pos(n, x,
-                                      y + self._VSPACING + brect(node).height())
+                x, ry = self._fix_pos(n, x, y + self._VSPACING + brect(node).height())
             x = (node.branches[0].pos().x() + node.branches[-1].pos().x()) / 2
             node.setPos(x, y)
-            for e in node.edges:
+            for e in node.graph_edges():
                 e.update_ends()
         else:
             node.setPos(self.gx, y)

Orange/widgets/visualize/tests/test_owtreegraph.py

@@ -1,9 +1,11 @@
 # Test methods with long descriptive names can omit docstrings
 # pylint: disable=missing-docstring
+from os import path
+
 from Orange.classification import TreeLearner
+from Orange.data import Table
 from Orange.widgets.tests.base import WidgetTest, WidgetOutputsTestMixin
-from Orange.widgets.visualize.owtreeviewer import \
-    OWTreeGraph
+from Orange.widgets.visualize.owtreeviewer import OWTreeGraph
 
 
 class TestOWTreeGraph(WidgetTest, WidgetOutputsTestMixin):
@@ -19,6 +21,12 @@ def setUpClass(cls):
         cls.signal_name = "Tree"
         cls.signal_data = cls.model
 
+        # Load a dataset that contains two variables with the same entropy
+        data_same_entropy = Table(path.join(
+            path.dirname(__file__), "../../tests/datasets/same_entropy.tab"))
+        cls.data_same_entropy = tree(data_same_entropy)
+        cls.data_same_entropy.instances = data_same_entropy
+
     def setUp(self):
         self.widget = self.create_widget(OWTreeGraph)
 
@@ -35,3 +43,43 @@ def test_target_class_changed(self):
         self.widget.color_combo.activated.emit(1)
         self.widget.color_combo.setCurrentIndex(1)
         self.assertNotEqual(nodes[0].toPlainText(), text)
+
+    def test_tree_determinism(self):
+        """Check that the tree is drawn identically upon receiving the same
+        dataset with no parameter changes."""
+        n_tries = 10
+
+        def _check_all_same(data):
+            """Check that all the elements within an iterable are identical."""
+            iterator = iter(data)
+            try:
+                first = next(iterator)
+            except StopIteration:
+                return True
+            return all(first == rest for rest in iterator)
+
+        # Make sure the tree are deterministic for iris
+        scene_nodes = []
+        for i in range(n_tries):
+            self.send_signal(self.signal_name, self.signal_data)
+            scene_nodes.append([n.pos() for n in self.widget.scene.nodes()])
+        for node_row in zip(*scene_nodes):
+            self.assertTrue(
+                _check_all_same(node_row),
+                "The tree was not drawn identically in the %d times it was "
+                "sent to widget after receiving the iris dataset." % n_tries
+            )
+
+        # Make sure trees are deterministic with data where some variables have
+        # the same entropy
+        scene_nodes = []
+        for i in range(n_tries):
+            self.send_signal(self.signal_name, self.data_same_entropy)
+            scene_nodes.append([n.pos() for n in self.widget.scene.nodes()])
+        for node_row in zip(*scene_nodes):
+            self.assertTrue(
+                _check_all_same(node_row),
+                "The tree was not drawn identically in the %d times it was "
+                "sent to widget after receiving a dataset with variables with "
+                "same entropy." % n_tries
+            )