[FIX] OWTreeViewer: Fix trees being displayed differently for same tree object #2067
Conversation
|
@astaric The tests are failing because apparently the |
|
You need to include it in Manifest and setup.py. Maybe you could move the dataset to where the other datasets reside, where this is already done? (https://github.com/biolab/orange3/tree/master/Orange/widgets/tests/datasets) |
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.
pavlin-policar
force-pushed
the
deterministic-trees
branch
from
6f7913b
to
d6504db
Compare
|
Thanks, I've moved it to the |
Codecov Report
@@ Coverage Diff @@
## master #2067 +/- ##
==========================================
+ Coverage 69.81% 69.83% +0.02%
==========================================
Files 315 315
Lines 53965 54015 +50
==========================================
+ Hits 37678 37724 +46
- Misses 16287 16291 +4Continue to review full report at Codecov.
|
pavlin-policar
force-pushed
the
deterministic-trees
branch
from
b62907a
to
09e2a99
Compare
| # Make sure trees are deterministic with data where some variables have | ||
| # the same entropy | ||
| data_same_entropy = Table(path.join( | ||
| path.dirname(__file__), "../../tests/datasets/same_entropy.tab")) |
There was a problem hiding this comment.
Use os.path.join() everywhere to make the test pass on Windos.
There was a problem hiding this comment.
Is there a cleaner solution than path.join(path.dirname(path.dirname(path.dirname(__file__))), "tests", "datasets", "same_entropy.tab")? Could I maybe put in ".." instead of path.dirname?
| @@ -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")) | |||
| first = next(iterator) | ||
| except StopIteration: | ||
| return True | ||
| return all(first == rest for rest in iterator) |
There was a problem hiding this comment.
Would return len(set(data)) == 1 work?
There was a problem hiding this comment.
Unfortunately not, because I'm comparing QPointFs which are an unhashable type.
Issue
OWTreeViewer was not deterministic in showing trees. This was not specific to trees where attributes had the same entropy or whatever the selection criteria was, but occured on any tree (even with iris).
Description of changes
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 getting an ordering.
I've written a test that draws the tree 10x and compares the node positions, but since the ordering of the nodes was completely random before, this is not guaranteed to fail in case the trees are drawn with random ordering. The ordering could happen to be the same 10x. This hasn't actually happened to me yet on iris, since there are more nodes that can fail there, but this could potentially happen.
I have no idea how to test this so that it would always fail, due to the random nature of the ordering.
Includes