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 4 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
Binary file modified docs/moving-pictures/empress-tree-tandem.qzv
View file
Open in desktop
Binary file not shown.
Binary file modified docs/moving-pictures/empress-tree.qzv
View file
Open in desktop
Binary file not shown.
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
5 changes: 5 additions & 0 deletions empress/support_files/js/empress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,11 @@ define([
* Initializes WebGL and then draws the tree
*/
Empress.prototype.initialize = function () {
// add default color/visible to all nodes
kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
for (var i = 1; i <= this._tree.size; i++) {
this._treeData[i].color = this.DEFAULT_COLOR;
this._treeData[i].visible = true;
}
this._drawer.initialize();
this._events.setMouseEvents();
var nodeNames = Object.keys(this._nameToKeys);
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 @@ -316,11 +316,7 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -329,11 +325,7 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -346,13 +338,9 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -361,11 +349,7 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -378,13 +362,9 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -393,11 +373,7 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -406,11 +382,7 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -423,13 +395,9 @@ def test_filter_unobserved_features_from_phylogeny(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 @@ -438,13 +406,9 @@ def test_filter_unobserved_features_from_phylogeny(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