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.

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

Shear UI #505

Merged
merged 75 commits into from
Jul 17, 2021
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
cbf4060
bp shear
kwcantrell Mar 1, 2021
227ec29
style fix
kwcantrell Mar 1, 2021
66c2e56
checkpoint
kwcantrell Mar 6, 2021
1183aa7
checkpoint
kwcantrell Mar 9, 2021
eafbea3
Finished TreeController
kwcantrell Mar 10, 2021
7301e82
tree-controller
kwcantrell Mar 17, 2021
888cf72
merged with master
kwcantrell Mar 17, 2021
f4f668f
fix doc issue
kwcantrell Mar 17, 2021
a408326
style fix
kwcantrell Mar 17, 2021
b043826
Apply suggestions from code review
kwcantrell Mar 19, 2021
caa916e
Merge branch 'master' of https://github.com/biocore/empress into tree…
kwcantrell Mar 19, 2021
f598928
fix style
kwcantrell Mar 19, 2021
5ca5809
addressed comments
kwcantrell Mar 23, 2021
89a4326
removed shear
kwcantrell Mar 23, 2021
654df69
start ui
kwcantrell Mar 24, 2021
d6aa515
finished basic shear ui
kwcantrell Mar 25, 2021
de01d93
fixed test issue
kwcantrell Mar 25, 2021
fcb6b5b
removed old comments
kwcantrell Mar 25, 2021
55f6f7e
merged master
kwcantrell Mar 25, 2021
b3acbd1
Apply suggestions from code review
kwcantrell Mar 30, 2021
263dd6a
started addressing PR suggestions
kwcantrell Mar 30, 2021
5bb8729
Merge branch 'shear-ui' of https://github.com/kwcantrell/empress into…
kwcantrell Mar 30, 2021
68d0d63
fixed style
kwcantrell Mar 30, 2021
603bf97
still addressing comments
kwcantrell Apr 1, 2021
31b5af3
fixed style
kwcantrell Apr 1, 2021
7f56e68
fix unselect all
kwcantrell Apr 1, 2021
9a9bb75
Fixed unselect all for circ layout
kwcantrell Apr 1, 2021
9dc26bc
fixed issue were tree disappears if all tips sheared
kwcantrell Apr 1, 2021
4445ebd
check point
kwcantrell Apr 1, 2021
15f81ab
fixed clade collapse
kwcantrell Apr 1, 2021
4630689
fixed style
kwcantrell Apr 1, 2021
73892fd
fixed test
kwcantrell Apr 1, 2021
3ca595e
fixed no biom error
kwcantrell Apr 2, 2021
66e5ded
checkpoint
kwcantrell Apr 6, 2021
9eb4243
performance improvemens
kwcantrell Apr 7, 2021
e3d128f
merged master
kwcantrell Apr 8, 2021
4b4d8e3
finished documenting
kwcantrell Apr 9, 2021
3d05f17
style fix
kwcantrell Apr 9, 2021
d909e83
added warning to shear
kwcantrell Apr 9, 2021
3b6d854
Major performance increase!
kwcantrell Apr 11, 2021
0803dc0
Apply suggestions from code review
kwcantrell Apr 12, 2021
cc754e9
fix style
kwcantrell Apr 12, 2021
3a2eede
Add warning message to shear panel
kwcantrell Apr 13, 2021
95392a5
starting addressing @fedarko's comments
kwcantrell May 4, 2021
45e2d49
addressed @fedarko's comments
kwcantrell May 4, 2021
3570a19
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell May 4, 2021
e78ecd1
fix test
kwcantrell May 4, 2021
c95adf8
Apply suggestions from code review
kwcantrell May 17, 2021
af71569
added more @fedarko's suggestions + stle fix
kwcantrell May 17, 2021
35a2aa9
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell May 28, 2021
928a2ce
address @fedarko's PR comments
kwcantrell May 28, 2021
6d4571a
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell Jun 6, 2021
6074e5d
fixed collapse issue
kwcantrell Jun 10, 2021
0b081ca
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell Jun 10, 2021
0bd1160
Apply suggestions from code review
kwcantrell Jun 10, 2021
d3c2a61
fixed callback for emperor selections
kwcantrell Jun 10, 2021
ac544fb
fixed style
kwcantrell Jun 10, 2021
55ca475
modified node circle text
kwcantrell Jun 10, 2021
aedb860
merged with master
kwcantrell Jun 11, 2021
90571be
disabled tabs during animation
kwcantrell Jun 11, 2021
149ecb5
fixed tests/lint issues
kwcantrell Jun 11, 2021
288a140
Apply suggestions from code review
kwcantrell Jun 29, 2021
7faa70a
modified message
kwcantrell Jun 29, 2021
cb085ba
addressed comments
kwcantrell Jun 29, 2021
8af9b93
fixed python test
kwcantrell Jun 29, 2021
c0fbd57
Merge branch 'master' of https://github.com/biocore/empress into pr50…
fedarko Jul 17, 2021
0505705
Remove extra " in side panel disabled message
fedarko Jul 17, 2021
924b938
rm another extra "
fedarko Jul 17, 2021
0bae2ce
space after "
fedarko Jul 17, 2021
953abd8
fix problem i introduced in the last commit ._.
fedarko Jul 17, 2021
8767132
MNT: RM \t chars, make e-d-*.js docs consistent
fedarko Jul 17, 2021
01d7022
DOC: Adjust tab-disabled prefix using isEmpirePlot
fedarko Jul 17, 2021
c57a5d2
DOC: Make uses of bold fonts consistent
fedarko Jul 17, 2021
00b7dee
STY: prettify
fedarko Jul 17, 2021
b664bfd
Cleaner toast msg for no-FM-b/c-shearing case
fedarko Jul 17, 2021
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
7 changes: 7 additions & 0 deletions empress/support_files/css/empress.css
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,13 @@ p.side-header button:hover,
white-space: nowrap;
}

.shear-layer-legend div {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why this class is duplicated compared to barplot-layer-legend I notice there's a div filter but any reason why these need to be two different classes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When an overflow occurs I wanted the title of the filter to always be in view. For example if you select "Level 7" for the shear filter, you can scroll and always see "Level 7". This occurs because .shear-layey-legend only applies to div elements within the shear-legends container. If I used barplot-layer-legend then the title of the filter would also scroll and if I added div to barplot-layer-legend then it would break the barplot legend.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's probably a better solution but that is what was easiest for me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A simple-ish solution might be creating a title element for each shear layer (for example, the Layer N text in barplot layers), and then not having a title at all of the stuff in the shear layer legend. Since each feature metadata column can correspond to at most one shear layer, the title we assign a shear layer could be Shear Layer: Level 4 or something like that?

image

Copy link
Collaborator

@fedarko fedarko Apr 30, 2021

Choose a reason for hiding this comment

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

@kwcantrell Would it be possible to make this change? (edit - whoops, looks like github lost track of the earlier comments -- see parent at https://github.com/biocore/empress/pull/505/files#r603485148)

I don't think it should require much if any additional work, and should reduce the amount of code complexity -- I think it would let us remove this duplicated CSS code.

This would involve storing the "Level N" text in an element with <h3 style="text-align: center;"> instead of in the legend. The code for this is adaptable from the barplot layer JS here.

We could also store the select / unselect all buttons in a <p> underneath this h3 tag, with the same style as the feature / sample metadata barplot toggle buttons (see code here). This should make the UI look nicer and would be more consistent with the barplot stuff, I think?

I made these changes in a local copy of the repo in a few minutes of editing, and the UI now looks as follows. It's a small change but I think the consistency will help users.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To make things simpler, I just added the title/buttons to the layer div rather than the legend div. The result gives the same effect but changes less code (and we can still remove the duplicated css).
Screenshot from 2021-05-03 17-59-54

max-height: 15em;
/* The side panel width is 450px */
max-width: 430px;
overflow: auto;
}

kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
.barplot-layer-legend {
max-height: 15em;
/* The side panel width is 450px */
Expand Down
199 changes: 159 additions & 40 deletions empress/support_files/js/empress.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,21 +391,32 @@ define([
);
// Rectangular
if (this._currentLayout === "Rectangular") {
data = LayoutsUtil.rectangularLayout(
this._tree.getTree(),
4020,
4020,
// since lengths for "ignoreLengths" are set by `lengthGetter`,
// we don't need (and should likely deprecate) the ignoreLengths
// option for the Layout functions since the layout function only
// needs to know lengths in order to layout a tree, it doesn't
// really need encapsulate all of the logic for determining
// what lengths it should lay out.
this.leafSorting,
undefined,
lengthGetter,
checkLengthsChange
);
// tree is just root
kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
if (this._tree.currentSize == 1) {
data = {
xCoord: [null, 0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid rewriting [null, 0] a bunch of times, and to make updating it in the future easier (e.g. if we ever switch to 0-indexing on these arrays), we may want to define this as a variable earlier in this function (e.g. NULL_AND_0 = [null, 0]) and then just refer to that. Or even make a helper function that takes a bunch of data key names (e.g. ["xCoord", "yCoord", "highestChildYr"]) and produces a map of keys to [null, 0]...

...Although, if we are going to disable shearing all tips from the tree, then I guess we could just remove this code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of setting the value of each component, I create the following helper function:

var dataForOnlyRoot = function(coordKeys) {
    var rootCoordData = {};
    _.each(coordKeys, function(key) {
        rootCoordData[key] = [null, 0];
    });
    return rootCoordData;
};

yCoord: [null, 0],
highestChildYr: [null, 0],
lowestChildYr: [null, 0],
yScalingFactor: [null, 0],
};
} else {
data = LayoutsUtil.rectangularLayout(
this._tree.getTree(),
4020,
4020,
// since lengths for "ignoreLengths" are set by `lengthGetter`,
// we don't need (and should likely deprecate) the ignoreLengths
// option for the Layout functions since the layout function only
// needs to know lengths in order to layout a tree, it doesn't
// really need encapsulate all of the logic for determining
// what lengths it should lay out.
this.leafSorting,
undefined,
lengthGetter,
checkLengthsChange
);
}
this._yrscf = data.yScalingFactor;
for (i of this._tree.postorderTraversal((includeRoot = true))) {
// remove old layout information
Expand All @@ -421,15 +432,30 @@ define([
j += 1;
}
} else if (this._currentLayout === "Circular") {
data = LayoutsUtil.circularLayout(
this._tree.getTree(),
4020,
4020,
this.leafSorting,
undefined,
lengthGetter,
checkLengthsChange
);
if (this._tree.currentSize == 1) {
data = {
// store new layout information
x0: [null, 0],
y0: [null, 0],
x1: [null, 0],
y1: [null, 0],
angle: [null, 0],
arcx0: [null, 0],
arcy0: [null, 0],
arcStartAngle: [null, 0],
arcEndAngle: [null, 0],
};
} else {
data = LayoutsUtil.circularLayout(
this._tree.getTree(),
4020,
4020,
this.leafSorting,
undefined,
lengthGetter,
checkLengthsChange
);
}
for (i of this._tree.postorderTraversal((includeRoot = true))) {
// remove old layout information
this._treeData[i].length = this._numOfNonLayoutParams;
Expand All @@ -449,14 +475,22 @@ define([
j += 1;
}
} else {
data = LayoutsUtil.unrootedLayout(
this._tree.getTree(),
4020,
4020,
undefined,
lengthGetter,
checkLengthsChange
);
if (this._tree.currentSize == 1) {
data = {
// store new layout information
xCoord: [null, 0],
yCoord: [null, 0],
};
} else {
data = LayoutsUtil.unrootedLayout(
this._tree.getTree(),
4020,
4020,
undefined,
lengthGetter,
checkLengthsChange
);
}
for (i of this._tree.postorderTraversal((includeRoot = true))) {
// remove old layout information
this._treeData[i].length = this._numOfNonLayoutParams;
Expand Down Expand Up @@ -2142,6 +2176,18 @@ define([
this.drawTree();
};

/**
*
*/
Empress.prototype.getUniqueSampleMetadataInfo = function (cat) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you mentioned docs were a work in progress, but this function in particular could really use some details -- I am unsure why exactly this has to be used in place of getObsBy (I get that this removes tips that have been sheared from the tree, but it isn't clear where this change is useful "downstream" later on in colorBySampleCat).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree documentation is definitely needed!

Reason this is used in place of getObsBy is that the BIOMTable object does not access to the tree so it is unable to remove the nodes and I don't want this to be "downstream" in colorBySampleCat so we can avoid having to implement this in future functions if they require sample metadata.

var obs = this._biom.getObsBy(cat);
var nodes = new Set([...this._tree.postorderTraversal()]);
_.each(obs, function (observations, key) {
obs[key] = observations.filter((x) => nodes.has(x));
});
kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
return obs;
};

/**
* Color the tree using sample metadata
*
Expand All @@ -2162,7 +2208,8 @@ define([
reverse = false
) {
var tree = this._tree;
var obs = this._biom.getObsBy(cat);
// var obs = this._biom.getObsBy(cat);
kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
var obs = this.getUniqueSampleMetadataInfo(cat);
var categories = Object.keys(obs);

// Assign colors to categories
Expand All @@ -2177,6 +2224,11 @@ define([
var cm = colorer.getMapRGB();
// colors for the legend
var keyInfo = colorer.getMapHex();
for (var key in keyInfo) {
if (obs[key].length === 0) {
delete keyInfo[key];
}
}
fedarko marked this conversation as resolved.
Show resolved Hide resolved

// shared by the following for loops
var i, j, category;
Expand Down Expand Up @@ -2232,6 +2284,9 @@ define([
* an array of the node name(s) with each value.
*/
Empress.prototype.getUniqueFeatureMetadataInfo = function (cat, method) {
// get nodes in tree
var nodes = new Set([...this._tree.postorderTraversal()]);

// In order to access feature metadata for a given node, we need to
// find the 0-based index in this._featureMetadataColumns that the
// specified f.m. column corresponds to. (We *could* get around this by
Expand All @@ -2258,15 +2313,16 @@ define([
var uniqueValueToFeatures = {};
_.each(fmObjs, function (mObj) {
_.mapObject(mObj, function (fmRow, node) {
var fmVal = fmRow[fmIdx];
if (!_.has(uniqueValueToFeatures, fmVal)) {
uniqueValueToFeatures[fmVal] = [];
}
// need to convert to integer
node = parseInt(node);
// This is loosely based on how BIOMTable.getObsBy() works.
var fmVal = fmRow[fmIdx];
if (_.has(uniqueValueToFeatures, fmVal)) {
uniqueValueToFeatures[fmVal].push(node);
} else {
uniqueValueToFeatures[fmVal] = [node];
if (!nodes.has(node)) {
return;
}
uniqueValueToFeatures[fmVal].push(node);
});
});

Expand Down Expand Up @@ -2329,9 +2385,14 @@ define([
);
// colors for drawing the tree
var cm = colorer.getMapRGB();

// colors for the legend
var keyInfo = colorer.getMapHex();

for (var key in keyInfo) {
if (uniqueValueToFeatures[key].length === 0) {
delete keyInfo[key];
}
}
// Do upwards propagation only if the coloring method is "tip"
if (method === "tip") {
obs = this._projectObservations(obs, false);
Expand Down Expand Up @@ -3565,5 +3626,63 @@ define([
}
};

/**
* This will shear/unshear
*/
Empress.prototype.shear = function (shearMap) {
this._tree.unshear();
var scope = this;
var removeNodes = new Set();

shearMap.forEach(function (values, cat) {
var fmInfo = scope.getUniqueFeatureMetadataInfo(cat, "tip");
var uniqueValueToFeatures = fmInfo.uniqueValueToFeatures;
_.each(values, function (val) {
var obs = uniqueValueToFeatures[val];
for (var node of obs) {
removeNodes.add(node);
}
});
});

// remove removeNodes
var allNodes = Array.from(Array(this._tree.size + 1).keys());
kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
allNodes.shift();

allNodes = new Set(allNodes);
var keepNodes = new Set(
[...allNodes].filter((x) => !removeNodes.has(x))
);

var keepNames = [];
for (var node of keepNodes) {
var name = this._tree.name(this._tree.postorderselect(node));
keepNames.push(name);
}

this._tree.shear(new Set(keepNames));
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding a method to the TreeController class that takes in a map and shears the tree. For example shearWithMap(shearMap). This would update the internal sheared tree and we can retrieve the names as needed. Perhaps even packing the !== null check in the getAllNames method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you mean here. @ElDeveloper do you mind elaborating a bit more? What exactly is the map in shearWithMap and what do you mean by packing the !== null check in getAllNames?

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that some of the shearing functionality in empress.js can be moved to tree-controller.js. Namely the part where you figure out which tip identifiers to shear. My suggestion is to take the data in shearMap (the argument that's currently being passed to Empress.shear) and pass that to TreeController for example TreeController.shearToMap (or shearWithMap up to you). That way additional checks like verifying that names aren't null, etc can all be done in TreeController rather than in Empress.

If this doesn't make sense, let me know and we can hop on a quick call.

var nodeNames = this._tree.getAllNames();
// Don't include nodes with the name null (i.e. nodes without a
// specified name in the Newick file) in the auto-complete.
nodeNames = nodeNames.filter((n) => n !== null);
this._events.autocomplete(nodeNames);

kwcantrell marked this conversation as resolved.
Show resolved Hide resolved
fedarko marked this conversation as resolved.
Show resolved Hide resolved
this.getLayoutInfo();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this is duplicated code from Empress.initialize(); as with below, could you make this into a separate function that both initialize() and shear() call?


// Undraw or redraw barplots as needed (assuming barplots are supported
// in the first place, of course; if no feature or sample metadata at
// all was passed then barplots are not available :()
if (!_.isNull(this._barplotPanel)) {
var supported = this._barplotPanel.updateLayoutAvailability(
this._currentLayout
);
if (!supported && this._barplotsDrawn) {
this.undrawBarplots();
} else if (supported && this._barplotPanel.enabled) {
this.drawBarplots();
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this is duplicated code from Empress.reLayout(); could you make this into a separate function that both reLayout() and shear() call, to avoid having duplicate code cluttering things up?

};

return Empress;
});
9 changes: 0 additions & 9 deletions empress/support_files/js/legend.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,8 @@ define(["jquery", "underscore", "util"], function ($, _, util) {
* @param {Object} info Color key information. This should map unique
* values (e.g. in sample or feature metadata) to
* their assigned color, expressed in hex format.
*
* @throws {Error} If info has no keys. This check is done before anything
* else is done in this function.
*/
Legend.prototype.addCategoricalKey = function (name, info) {
if (_.isEmpty(info)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the error check being removed?

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 removed to handle cases where all tips are removed.

For example:

clicking on k__Archaea leads to:

throw new Error(
"Can't create a categorical legend when there are no " +
"categories in the info"
);
}
this.clear();
this.addTitle(name);
this._sortedCategories = util.naturalSort(_.keys(info));
Expand Down
Loading