-
Notifications
You must be signed in to change notification settings - Fork 31
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
Shear UI #505
Changes from 40 commits
Commits
Show all changes
75 commits
Select commit
Hold shift + click to select a range
cbf4060
bp shear
kwcantrell 227ec29
style fix
kwcantrell 66c2e56
checkpoint
kwcantrell 1183aa7
checkpoint
kwcantrell eafbea3
Finished TreeController
kwcantrell 7301e82
tree-controller
kwcantrell 888cf72
merged with master
kwcantrell f4f668f
fix doc issue
kwcantrell a408326
style fix
kwcantrell b043826
Apply suggestions from code review
kwcantrell caa916e
Merge branch 'master' of https://github.com/biocore/empress into tree…
kwcantrell f598928
fix style
kwcantrell 5ca5809
addressed comments
kwcantrell 89a4326
removed shear
kwcantrell 654df69
start ui
kwcantrell d6aa515
finished basic shear ui
kwcantrell de01d93
fixed test issue
kwcantrell fcb6b5b
removed old comments
kwcantrell 55f6f7e
merged master
kwcantrell b3acbd1
Apply suggestions from code review
kwcantrell 263dd6a
started addressing PR suggestions
kwcantrell 5bb8729
Merge branch 'shear-ui' of https://github.com/kwcantrell/empress into…
kwcantrell 68d0d63
fixed style
kwcantrell 603bf97
still addressing comments
kwcantrell 31b5af3
fixed style
kwcantrell 7f56e68
fix unselect all
kwcantrell 9a9bb75
Fixed unselect all for circ layout
kwcantrell 9dc26bc
fixed issue were tree disappears if all tips sheared
kwcantrell 4445ebd
check point
kwcantrell 15f81ab
fixed clade collapse
kwcantrell 4630689
fixed style
kwcantrell 73892fd
fixed test
kwcantrell 3ca595e
fixed no biom error
kwcantrell 66e5ded
checkpoint
kwcantrell 9eb4243
performance improvemens
kwcantrell e3d128f
merged master
kwcantrell 4b4d8e3
finished documenting
kwcantrell 3d05f17
style fix
kwcantrell d909e83
added warning to shear
kwcantrell 3b6d854
Major performance increase!
kwcantrell 0803dc0
Apply suggestions from code review
kwcantrell cc754e9
fix style
kwcantrell 3a2eede
Add warning message to shear panel
kwcantrell 95392a5
starting addressing @fedarko's comments
kwcantrell 45e2d49
addressed @fedarko's comments
kwcantrell 3570a19
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell e78ecd1
fix test
kwcantrell c95adf8
Apply suggestions from code review
kwcantrell af71569
added more @fedarko's suggestions + stle fix
kwcantrell 35a2aa9
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell 928a2ce
address @fedarko's PR comments
kwcantrell 6d4571a
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell 6074e5d
fixed collapse issue
kwcantrell 0b081ca
Merge branch 'master' of https://github.com/biocore/empress into shea…
kwcantrell 0bd1160
Apply suggestions from code review
kwcantrell d3c2a61
fixed callback for emperor selections
kwcantrell ac544fb
fixed style
kwcantrell 55ca475
modified node circle text
kwcantrell aedb860
merged with master
kwcantrell 90571be
disabled tabs during animation
kwcantrell 149ecb5
fixed tests/lint issues
kwcantrell 288a140
Apply suggestions from code review
kwcantrell 7faa70a
modified message
kwcantrell cb085ba
addressed comments
kwcantrell 8af9b93
fixed python test
kwcantrell c0fbd57
Merge branch 'master' of https://github.com/biocore/empress into pr50…
fedarko 0505705
Remove extra " in side panel disabled message
fedarko 924b938
rm another extra "
fedarko 0bae2ce
space after "
fedarko 953abd8
fix problem i introduced in the last commit ._.
fedarko 8767132
MNT: RM \t chars, make e-d-*.js docs consistent
fedarko 01d7022
DOC: Adjust tab-disabled prefix using isEmpirePlot
fedarko c57a5d2
DOC: Make uses of bold fonts consistent
fedarko 00b7dee
STY: prettify
fedarko b664bfd
Cleaner toast msg for no-FM-b/c-shearing case
fedarko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,7 +131,8 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
* performance boost | ||
*/ | ||
var eCache = []; | ||
for (var i = 0; i < this.b_.length; i++) { | ||
var i; | ||
for (i = 0; i < this.b_.length; i++) { | ||
eCache.push(2 * this.r1Cache_[i] - i - 1); | ||
} | ||
this.eCache_ = new Uint32Array(eCache); | ||
|
@@ -190,6 +191,33 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
* that have the same name. | ||
*/ | ||
this._nameToNodes = {}; | ||
|
||
/** | ||
* @type {Array} | ||
* @private | ||
* Cache Parent nodes. This will help speed up a lot of methods that | ||
* make calls to parent. | ||
*/ | ||
this._pCache = []; | ||
var pStack = []; | ||
for (i = 0; i < this.b_.length - 1; i++) { | ||
if (this.b_[i] === 1) { | ||
if (pStack.length === 0) { | ||
pStack.push(i); | ||
this._pCache[i] = 0; | ||
} else if (this.b_[i + 1] === 1) { | ||
this._pCache[i] = pStack[pStack.length - 1]; | ||
pStack.push(i); | ||
} else if (this.b_[i + 1] === 0) { | ||
this._pCache[i] = pStack[pStack.length - 1]; | ||
} | ||
} else if (this.b_[i] === 0) { | ||
this._pCache[i] = pStack[pStack.length - 1]; | ||
if (this.b_[i + 1] === 0) { | ||
pStack.pop(); | ||
} | ||
} | ||
Comment on lines
+203
to
+219
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this block be moved to a helper method? Seems a bit odd that it's in the init method. |
||
} | ||
} | ||
|
||
/** | ||
|
@@ -452,7 +480,7 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
return -1; | ||
} | ||
|
||
return this.enclose(i); | ||
return this._pCache[i]; | ||
}; | ||
|
||
/** | ||
|
@@ -702,7 +730,9 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
// find first and last preorder positions of the subtree spanned | ||
// by the current internal node | ||
var n = this.postorderselect(nodeKey); | ||
if (this.isleaf(n)) { | ||
|
||
// check for root to account for cases when all tips have been sheared | ||
if (this.isleaf(n) && nodeKey !== this.postorder(this.root())) { | ||
throw "Error: " + nodeKey + " is a tip!"; | ||
} | ||
var start = this.preorder(this.fchild(n)); | ||
|
@@ -984,41 +1014,58 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
* convert the original postorder positions to the sheared | ||
* tree postorder positions ("newToOld") and vice-versa ("oldToNew"). | ||
*/ | ||
BPTree.prototype.shear = function (keepTips) { | ||
BPTree.prototype.shear = function (removeTips) { | ||
fedarko marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// closure | ||
var scope = this; | ||
|
||
// create new names and lengths array | ||
var names = [null]; | ||
var lengths = [null]; | ||
|
||
// create new bit array | ||
var mask = []; | ||
var mask = _.clone(this.b_); | ||
|
||
// function to that will set open/close bits for a node | ||
var set_bits = (node) => { | ||
mask[node] = 1; | ||
mask[scope.close(node)] = 0; | ||
}; | ||
var i, node; | ||
|
||
// set root open/close bits | ||
set_bits(this.root()); | ||
// remove tips | ||
for (i of removeTips) { | ||
node = this.postorderselect(i); | ||
mask[node] = undefined; | ||
mask[node + 1] = undefined; | ||
} | ||
|
||
// iterate over bp tree in post order and add all tips that are in | ||
// keepTips plus their ancestors | ||
var i; | ||
for (i = 1; i <= this.size; i++) { | ||
var node = this.postorderselect(i); | ||
var name = this.name(node); | ||
if (this.isleaf(node) && keepTips.has(name)) { | ||
// set open/close bits for tip | ||
set_bits(node); | ||
|
||
// set open/close bits for tips ancestors | ||
var parent = this.parent(node); | ||
while (parent !== this.root() || mask[parent] !== 1) { | ||
set_bits(parent); | ||
parent = this.parent(parent); | ||
// remove internal | ||
var nodeStack = []; | ||
for (i = mask.length - 1; i > 0; i--) { | ||
if (mask[i] === 0) { | ||
// close parentheses | ||
if (mask[i - 1] === 0) { | ||
// close parentheses represents internal node | ||
nodeStack.push([i, null]); | ||
} else if (mask[i - 1] === 1) { | ||
// close parentheses represents non-removed tip | ||
// thus we mark it as false so it is not removed | ||
nodeStack.push([i, false]); | ||
} else if (mask[i - 1] === undefined) { | ||
// close paretheses represents an internal node | ||
kwcantrell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// with at least one removed tip so we temporarly mark it | ||
// as true to remove | ||
nodeStack.push([i, true]); | ||
} | ||
} else if (mask[i] === 1) { | ||
//open parentheses | ||
kwcantrell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
node = nodeStack.pop(); | ||
if (node[1] === true) { | ||
// remove internal node | ||
mask[i] = undefined; | ||
mask[node[0]] = undefined; | ||
} else if (node[1] === false) { | ||
// need explicitly check false since it can be null | ||
// if node[1] is false that means it contains a tip and thus | ||
// we need to mark its parent as false so it is not removed | ||
nodeStack[nodeStack.length - 1][1] = false; | ||
} | ||
|
||
if (nodeStack[nodeStack.length - 1][1] === null) { | ||
// if null then we set its remove status to whatever node | ||
// was | ||
nodeStack[nodeStack.length - 1][1] = node[1]; | ||
} | ||
} | ||
} | ||
|
@@ -1027,14 +1074,16 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
var shearedToFull = new Map(); | ||
var fullToSheared = new Map(); | ||
var postorderPos = 1; | ||
// create new names and lengths array | ||
var names = [null]; | ||
var lengths = [null]; | ||
for (i = 0; i < mask.length; i++) { | ||
if (mask[i] !== undefined) { | ||
newBitArray.push(mask[i]); | ||
} | ||
|
||
// get name and length of node | ||
// Note: names and lengths of nodes are stored in postorder | ||
|
||
if (mask[i] === 0) { | ||
names.push(this.name(i)); | ||
lengths.push(this.length(i)); | ||
|
@@ -1043,6 +1092,7 @@ define(["ByteArray", "underscore"], function (ByteArray, _) { | |
postorderPos += 1; | ||
} | ||
} | ||
|
||
return { | ||
shearedToFull: shearedToFull, | ||
fullToSheared: fullToSheared, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 adiv
filter but any reason why these need to be two different classes?There was a problem hiding this comment.
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 todiv
elements within theshear-legends
container. If I usedbarplot-layer-legend
then the title of the filter would also scroll and if I added div tobarplot-layer-legend
then it would break the barplot legend.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 beShear Layer: Level 4
or something like that?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).