Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

163 remove tree data fix #231

Merged
merged 10 commits into from
Jul 7, 2020
Merged

163 remove tree data fix #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5693c3c
removed tree data before name
kwcantrell Jul 2, 2020
b53fee2
removed redundant tree_data
kwcantrell Jul 2, 2020
b1a1abf
fix comment
kwcantrell Jul 2, 2020
d56df15
fixed travis test
kwcantrell Jul 2, 2020
cd7b373
moved treeData color init
kwcantrell Jul 7, 2020
fb40527
checkpoint-before tree_data refactor
kwcantrell Jul 7, 2020
5f879ef
treedata
kwcantrell Jul 7, 2020
fe8ae6d
Revert "treedata"
kwcantrell Jul 7, 2020
9cf9fa7
resolved conflicts
kwcantrell Jul 7, 2020
578b409
fixed tests
kwcantrell Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions empress/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,11 @@ def _to_dict(self):

tree_data = {}
names_to_keys = {}
# Note: tree_data starts with index 1 because the bp tree uses 1 based
# indexing
for i, node in enumerate(self.tree.postorder(include_self=True), 1):
tree_data[i] = {
'name': node.name,
'color': [0.75, 0.75, 0.75],
'sampVal': 1,
'visible': True,
'single_samp': False
}
# Add coordinate data from all layouts for this node
for layoutsuffix in layout_to_coordsuffix.values():
Expand Down
16 changes: 11 additions & 5 deletions empress/support_files/js/empress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,6 @@ define([
*/
this._tree = tree;
this._numTips = 0;
for (var i = 0; i < this._tree.size; i++) {
if (this._tree.isleaf(this._tree.postorderselect(i))) {
this._numTips++;
}
}

/**
* @type {Object}
Expand All @@ -105,6 +100,17 @@ define([
*/
this._treeData = treeData;

// count number of tips and set default color/visible
// Note: currently empress tree uses 1-based index since the bp-tree
// bp-tree.js is based off of used 1-based index.
for (var i = 1; i <= this._tree.size; i++) {
this._treeData[i].color = this.DEFAULT_COLOR;
this._treeData[i].visible = true;
if (this._tree.isleaf(this._tree.postorderselect(i))) {
this._numTips++;
}
}

/**
* @type{Object}
* Converts tree node names to an array of _treeData keys.
Expand Down
48 changes: 6 additions & 42 deletions tests/python/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,11 +373,7 @@ def test_no_intersection_between_tree_and_table(self):
'int_metadata': {},
'feature_metadata_columns': [],
'tree': [1, 1, 1, 1, 0, 1, 0, 0, 1, 0, 0, 1, 1, 0, 1, 0, 0, 0],
'tree_data': {1: {'color': [0.75, 0.75, 0.75],
'name': 'a',
'sampVal': 1,
'single_samp': False,
'visible': True,
'tree_data': {1: {'name': 'a',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwcantrell would it be hard to replace the dictionary of dictionaries for a list of lists? It seems like the key for tree data is an index eitherway. In addition, if we can change this:

{'name': 'a', 'x2': 1, 'xc0': 2, 'xc1': 3, 'xr': 4, 'y2': 5, 'yc0': 6, 'yc1': 7, 'yr': 8}

To this:

['a', 1, 2, 3, 4, 5, 6, 7, 8]

This should save python from writing all the repeated key data, and save JavaScript from parsing all of that. We would just need to load the data in a container once it's parsed in JavaScript. If this is straightforward to do, then let's do it in this PR otherwise we can defer to a later PR since this goes beyond the original scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good suggestion @ElDeveloper. I'll see if I can change treeData to a list of lists. However, I will need to modify a fair amount of the empress class so we should wait to merge this PR until #230 is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's defer this until later then.

'x2': -82.19088834200284,
'xc0': 1481.4675640601124,
'xc1': 2222.2013460901685,
Expand All @@ -386,11 +382,7 @@ def test_no_intersection_between_tree_and_table(self):
'yc0': 0.0,
'yc1': 0.0,
'yr': -2386.875},
2: {'color': [0.75, 0.75, 0.75],
'name': 'e',
'sampVal': 1,
'single_samp': False,
'visible': True,
2: {'name': 'e',
'x2': 948.7236134182863,
'xc0': 457.7986539098309,
'xc1': 915.5973078196618,
Expand All @@ -403,13 +395,9 @@ def test_no_intersection_between_tree_and_table(self):
'arcstartangle': 1.2566370614359172,
'arcx0': 457.7986539098309,
'arcy0': 1408.9593804792778,
'color': [0.75, 0.75, 0.75],
'highestchildyr': -1381.875,
'lowestchildyr': -2386.875,
'name': 'EmpressNode0',
'sampVal': 1,
'single_samp': False,
'visible': True,
'x2': 295.3117872853636,
'xc0': 599.2662179699436,
'xc1': 1198.5324359398871,
Expand All @@ -418,11 +406,7 @@ def test_no_intersection_between_tree_and_table(self):
'yc0': 435.3923929520944,
'yc1': 870.7847859041888,
'yr': -1884.375},
4: {'color': [0.75, 0.75, 0.75],
'name': 'b',
'sampVal': 1,
'single_samp': False,
'visible': True,
4: {'name': 'b',
'x2': 1485.5419815224768,
'xc0': -599.2662179699435,
'xc1': -1797.7986539098304,
Expand All @@ -435,13 +419,9 @@ def test_no_intersection_between_tree_and_table(self):
'arcstartangle': 2.5132741228718345,
'arcx0': -599.2662179699435,
'arcy0': 435.3923929520945,
'color': [0.75, 0.75, 0.75],
'highestchildyr': -376.875,
'lowestchildyr': -1884.375,
'name': 'g',
'sampVal': 1,
'single_samp': False,
'visible': True,
'x2': 326.7059130664611,
'xc0': 0.0,
'xc1': 4.5356862759171076e-14,
Expand All @@ -450,11 +430,7 @@ def test_no_intersection_between_tree_and_table(self):
'yc0': 0.0,
'yc1': 740.7337820300562,
'yr': -1130.625},
6: {'color': [0.75, 0.75, 0.75],
'name': 'EmpressNode1',
'sampVal': 1,
'single_samp': False,
'visible': True,
6: {'name': 'EmpressNode1',
'x2': -622.0177003518252,
'xc0': -1198.5324359398871,
'xc1': -1797.7986539098308,
Expand All @@ -463,11 +439,7 @@ def test_no_intersection_between_tree_and_table(self):
'yc0': -870.7847859041887,
'yc1': -1306.177178856283,
'yr': 628.125},
7: {'color': [0.75, 0.75, 0.75],
'name': 'd',
'sampVal': 1,
'single_samp': False,
'visible': True,
7: {'name': 'd',
'x2': -2333.458018477523,
'xc0': 457.79865390983053,
'xc1': 1144.4966347745763,
Expand All @@ -480,13 +452,9 @@ def test_no_intersection_between_tree_and_table(self):
'arcstartangle': 5.026548245743669,
'arcx0': 457.79865390983053,
'arcy0': -1408.9593804792778,
'color': [0.75, 0.75, 0.75],
'highestchildyr': 1633.125,
'lowestchildyr': 628.125,
'name': 'h',
'sampVal': 1,
'single_samp': False,
'visible': True,
'x2': -653.4118261329227,
'xc0': -0.0,
'xc1': -457.79865390983105,
Expand All @@ -495,13 +463,9 @@ def test_no_intersection_between_tree_and_table(self):
'yc0': -0.0,
'yc1': -1408.9593804792778,
'yr': 1130.625},
9: {'color': [0.75, 0.75, 0.75],
'highestchildyr': 1130.625,
9: {'highestchildyr': 1130.625,
'lowestchildyr': -1130.625,
'name': 'EmpressNode2',
'sampVal': 1,
'single_samp': False,
'visible': True,
'x2': 0.0,
'xc0': 0.0,
'xc1': 0.0,
Expand Down
49 changes: 24 additions & 25 deletions tests/test-empress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,36 +274,36 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
// Note: node 6's name is EmpressNode6 which means it will not be
// included in the getNodeCoords()
var rectCoords = new Float32Array([
1, 2, 1.0, 1.0, 1.0,
3, 4, 1.0, 1.0, 1.0,
5, 6, 1.0, 1.0, 1.0,
7, 8, 1.0, 1.0, 1.0,
9, 10, 1.0, 1.0, 1.0,
13, 14, 1.0, 1.0, 1.0
1, 2, 0.75, 0.75, 0.75,
3, 4, 0.75, 0.75, 0.75,
5, 6, 0.75, 0.75, 0.75,
7, 8, 0.75, 0.75, 0.75,
9, 10, 0.75, 0.75, 0.75,
13, 14, 0.75, 0.75, 0.75,
]);
this.empress._currentLayout = "Rectangular";
var empressRecCoords = this.empress.getNodeCoords();
deepEqual(empressRecCoords, rectCoords);

var circCoords = new Float32Array([
15, 16, 1.0, 1.0, 1.0,
17, 18, 1.0, 1.0, 1.0,
19, 20, 1.0, 1.0, 1.0,
21, 22, 1.0, 1.0, 1.0,
23, 24, 1.0, 1.0, 1.0,
27, 28, 1.0, 1.0, 1.0,
15, 16, 0.75, 0.75, 0.75,
17, 18, 0.75, 0.75, 0.75,
19, 20, 0.75, 0.75, 0.75,
21, 22, 0.75, 0.75, 0.75,
23, 24, 0.75, 0.75, 0.75,
27, 28, 0.75, 0.75, 0.75,
]);
this.empress._currentLayout = "Circular";
var empressCirCoords = this.empress.getNodeCoords();
deepEqual(empressCirCoords, circCoords);

var unrootCoords = new Float32Array([
29, 30, 1.0, 1.0, 1.0,
31, 32, 1.0, 1.0, 1.0,
33, 34, 1.0, 1.0, 1.0,
35, 36, 1.0, 1.0, 1.0,
37, 38, 1.0, 1.0, 1.0,
41, 42, 1.0, 1.0, 1.0,
29, 30, 0.75, 0.75, 0.75,
31, 32, 0.75, 0.75, 0.75,
33, 34, 0.75, 0.75, 0.75,
35, 36, 0.75, 0.75, 0.75,
37, 38, 0.75, 0.75, 0.75,
41, 42, 0.75, 0.75, 0.75,
]);
this.empress._currentLayout = "Unrooted";
var empressUnrootCoords = this.empress.getNodeCoords();
Expand Down Expand Up @@ -339,7 +339,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function

test("Test colorSampleIDs", function() {
var samples = ["s2","s7"];
var defaultColor = [1.0, 1.0, 1.0];
var defaultColor = [0.75, 0.75, 0.75];
var color = [0.5, 0.6, 0.7];
this.empress.colorSampleIDs(samples, color);

Expand All @@ -364,8 +364,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
};
this.empress.colorSampleGroups(sampleGroup);

// the entire tree should be colored the samples in sampleGroup
// contain all tips
// the entire tree should be colored. sampleGroup contain all tips
for (var i = 1; i <=7; i++) {
var node = this.empress._treeData[i];
deepEqual(node.color, [1.0, 0, 0]);
Expand All @@ -392,7 +391,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
} else if(greeNodes.has(i)) {
deepEqual(node.color, [0, 1.0, 0]);
} else {
deepEqual(node.color, [1.0, 1.0, 1.0]);
deepEqual(node.color, [0.75, 0.75, 0.75]);
}
}
});
Expand Down Expand Up @@ -454,7 +453,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
chroma(cm["a"]).gl().slice(0, 3)
);
} else {
deepEqual(node.color, [1.0, 1.0, 1.0]);
deepEqual(node.color, [0.75, 0.75, 0.75]);
}
}

Expand Down Expand Up @@ -496,7 +495,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
chroma(cm["2"]).gl().slice(0, 3)
);
} else {
deepEqual(node.color, [1.0, 1.0, 1.0]);
deepEqual(node.color, [0.75, 0.75, 0.75]);
}
}

Expand Down Expand Up @@ -527,7 +526,7 @@ require(["jquery", "BPTree", "Empress", "BiomTable", "util", "chroma"], function
chroma(cm["2"]).gl().slice(0, 3)
);
} else {
deepEqual(node.color, [1.0, 1.0, 1.0]);
deepEqual(node.color, [0.75, 0.75, 0.75]);
}
}

Expand Down