Node default #486
Node default #486
Conversation
There was a problem hiding this comment.
@kwcantrell, thanks so much. This looks good but I think we can reduce the number of circles drawn. Instead of drawing all internal node circles, just show node circles for internal nodes that have only 1 descendant. This will help avoid issues where a branch looks "long" but is really a series of nodes one after the other. It will also reduce overhead by default drawing even fewer nodes.
See the example I posted here you'll see there's a few branches with a lot of internal nodes that have only one descendant.
| if (this.drawNodeCircles === 0) { | ||
| comp = function (node) { | ||
| return ( | ||
| visible(node) && !tree.isleaf(tree.postorderselect(node)) |
There was a problem hiding this comment.
This is pseudo-code but I think this would help further reduce the number of circles drawn on screen:
| visible(node) && !tree.isleaf(tree.postorderselect(node)) | |
| visible(node) && !tree.isleaf(tree.postorderselect(node)) && tree.children(...).length === 1``` |
There was a problem hiding this comment.
I added a forth option (show internal nodes with one descendant) and made it the default. Here is an example.
There was a problem hiding this comment.
Excellent, that looks exactly like what we need! Just for completeness how does that clade in the tree look when you show all node circles?
There was a problem hiding this comment.
Here is the clade. The node highlighted in the above clade is located on the bottom branch.
There was a problem hiding this comment.
Perfect, this looks great! Whenever you get a chance feel free to push the code and I'll be happy to re-review the code.
There was a problem hiding this comment.
my bad. I had commited it but forgot to push it.
| @@ -105,6 +107,43 @@ require([ | |||
| deepEqual(empressUnrootCoords, unrootCoords); | |||
| }); | |||
|
|
|||
| test("Test getNodeCoords only internal node circles", function () { | |||
| // have empress draw only internal node circles | |||
| this.empress.drawNodeCircles = 0; | |||
There was a problem hiding this comment.
Can you add a test for the case where this value is 3 i.e. the default?
empress/support_files/js/empress.js
Outdated
| * each node's position. | ||
| * @param{String} showTreeNodes 0 - draw only internal nodes circles, | ||
| * 1 - draw all node circles, | ||
| * 2 - hide all node circles |
There was a problem hiding this comment.
Can you add the missing value here?
There was a problem hiding this comment.
added missing value
| return scope.getNodeInfo(node, "visible"); | ||
| }; | ||
| var comp; | ||
| if (this.drawNodeCircles === 0) { |
There was a problem hiding this comment.
Can you add a comment before each if describing what case this is?
There was a problem hiding this comment.
add comment to each if case
|
thanks @ElDeveloper! The PR should be good to merge in now. |
This PR address #479. I added new node circle option "Show internal node circles" and made it the default option.