Tree controller #492

kwcantrell · 2021-03-17T02:24:12Z

This PR implements a new object TreeController that will allow empress to support dynamic shearing based on feature metadata or even sample metadata. TreeController will shear the tree based on a set of tip names (based on https://github.com/wasade/improved-octo-waddle/blob/0e9e75b77238acda6752f59d940620f89607ba6b/bp/_bp.pyx#L732). Also, to make integration of TreeController as seamless as possible, all input and output of TreeController will be w.r.t the original tree. In other words empress.js will not know if a tree has been sheared. Instead, TreeController will internally convert its input from the original tree to the sheared tree and back. This makes handling the metadata A LOT easier since empress indexes metadata using a nodes post-order position (w.r.t the non-sheared tree).

In addition, TreeController introduces iterators for the post-order and in-order traversals (now we no longer need to do for (i=1; i < this.tree.size; i++) to perform post-order traversal. Instead, we simply do for(node of this.tree.postorderTraversal()))

I've gone through a lot of different iterations of this PR so I am not sure how simple this final product turned out. So we can also setup a meeting if it makes reviewing this PR easier. 🙃

ElDeveloper

This is amazing. I am having quite a lot of fun with it. For those curious about it:

Recoloring needs to happen manually, etc but I think that's a glitch outside of the scope of this PR.

empress/support_files/js/bp-tree.js

ElDeveloper · 2021-03-17T19:53:00Z

empress/support_files/js/bp-tree.js

+        var newToOld = {};
+        var oldToNew = {};


Maybe this is a good opportunity to use the Map object?

empress/support_files/js/empress.js

ElDeveloper · 2021-03-17T19:58:14Z

empress/support_files/js/empress.js

                this._treeData[i][this._tdToInd.arcendangle] =
-                    data.arcEndAngle[i];
+                    data.arcEndAngle[j++];


Can you put the j increment in a separate line? Seems a bit "hidden" this way.

ElDeveloper · 2021-03-17T21:30:39Z

tests/test-bp-tree.js

@@ -929,7 +929,7 @@ require(["jquery", "ByteArray", "BPTree"], function ($, ByteArray, BPTree) {
            );

            var keep = new Set(["4", "6", "7", "10", "11"]);
-            var result = preShearBPTree.shear(keep);
+            var result = preShearBPTree.shear(keep).tree;


Can you add tests for the oldToNew and newToOld maps?

ElDeveloper · 2021-03-17T21:34:47Z

empress/support_files/js/tree-controller.js

+
+        // initialize
+        for (var i = 1; i <= this.currentTree.size; i++) {
+            this.origToCur[i] = i;


I suggest keeping the names consistent with the other classes.

This is just a suggestion but maybe consider these alternative names:

"fullToSheared" and "shearedToFull"

"pastToPresent" and "presentToPast"

Same renaming would need to apply to the properties for example this.full = tree and this.sheared = this.full.shear(tips) ....

ElDeveloper · 2021-03-17T21:50:55Z

empress/support_files/js/tree-controller.js

+     *                  to Numbers representing the minimum, maximum, and
+     *                  average non-root node length in the tree.
+     */
+    TreeController.prototype.getLengthStats = function () {


Thanks for documenting this class so clearly. Although I am having a bit of a hard time figuring out when the methods refer to the current or to the original tree. What do you think about writing a property for "current" and "original" as part of the controller so that their methods are directly reused from those objects. Instead of having to replicate each method at this class' level.

For example using JavaScript's defineProperty. For example

Object.defineProperty(this, 'currentTree', { value: this.model.currentTree, writable: false });

Other alternative I can think of is making the names of the methods more expressive. For example TreeController.prototype.getCurrentLengthStats instead of just getLengthStats.

Although I am having a bit of a hard time figuring out when the methods refer to the current or to the original tree.

In a way this is the point of TreeController. From a users perspective, everything is in relation to the "original" tree which is why each method from BPTree is replicated on the class level of TreeController. Essentially, I wanted empress to be completely unaware of tree shearing so that additional logic does not need to be added to empress.js to account for tree shearing.

The tldr is that empress is setup in such a way that it only interacts with tree through the various traversal methods (which handles the conversion between the sheared tree and original tree automatically). However, if you want more details, we can setup a quick meeting as it would be a lot easier to show you.

That makes sense, thanks for explaining. I think the only methods that threw me off were those related to "names" which I think are a special case based on our offline discussion.

empress/support_files/js/empress.js

kwcantrell · 2021-03-18T00:47:42Z

Thanks @ElDeveloper for posting a live demonstration! Like you said this PR is not meant to handle recoloring, the legend, etc. That will all be handled in a separate PR that deals with the UI.

gwarmstrong · 2021-03-18T01:26:50Z

empress/support_files/js/empress.js

        // set up length getter
        var branchMethod = this.branchMethod;
        var checkLengthsChange = LayoutsUtil.shouldCheckBranchLengthsChanged(
            branchMethod
        );
        var lengthGetter = LayoutsUtil.getLengthMethod(
            branchMethod,
-            this._tree
+            this._tree.model.currentTree


You may want to expose a method for the TreeController that exposes the tree of the model. Having large chains where you need to know the class of an attributes' attributes, the system can unintendedly get somewhat complex.

gwarmstrong · 2021-03-18T01:51:44Z

This looks awesome! Am I correct that it is not hooked up to the interface in any way yet?

ElDeveloper · 2021-03-18T02:17:15Z

Yep, you are correct! Thanks for looking over.

…

On (Mar-17-21|18:52), George Armstrong wrote: This looks awesome! Am I correct that it is not hooked up to the interface in any way yet? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://urldefense.com/v3/__https://github.com/biocore/empress/pull/492*issuecomment-801555810__;Iw!!Mih3wA!UkuNGX4xbqrOvyQKpeO9dDvnOOIhbdBgf6lurjSopkj5sQwbbOzCIcfLZAwf8uQ$

@ElDeveloper

merged @ElDeveloper's suggestions Co-authored-by: Yoshiki Vázquez Baeza <yoshiki@ucsd.edu>

…-controller

emperor-helper · 2021-03-19T01:07:36Z

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv

ElDeveloper

Thanks so much @kwcantrell!

empress/support_files/js/tree-controller.js

ElDeveloper · 2021-03-25T20:53:14Z

Thanks so much @kwcantrell !

kwcantrell added 8 commits March 1, 2021 11:25

bp shear

cbf4060

style fix

227ec29

checkpoint

66c2e56

checkpoint

1183aa7

Finished TreeController

eafbea3

tree-controller

7301e82

merged with master

888cf72

fix doc issue

f4f668f

ElDeveloper requested a review from gwarmstrong March 17, 2021 02:43

style fix

a408326

ElDeveloper approved these changes Mar 18, 2021

View reviewed changes

gwarmstrong reviewed Mar 18, 2021

View reviewed changes

kwcantrell and others added 3 commits March 18, 2021 17:55

Apply suggestions from code review

b043826

merged @ElDeveloper's suggestions Co-authored-by: Yoshiki Vázquez Baeza <yoshiki@ucsd.edu>

Merge branch 'master' of https://github.com/biocore/empress into tree…

caa916e

…-controller

fix style

f598928

kwcantrell added 2 commits March 23, 2021 14:43

addressed comments

5ca5809

removed shear

89a4326

ElDeveloper approved these changes Mar 24, 2021

View reviewed changes

empress/support_files/js/tree-controller.js Outdated Show resolved Hide resolved

renamed function

a003ab9

kwcantrell mentioned this pull request Mar 25, 2021

Shear UI #505

Merged

ElDeveloper merged commit 1939236 into biocore:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree controller #492

Tree controller #492

kwcantrell commented Mar 17, 2021 •

edited

ElDeveloper left a comment

ElDeveloper Mar 17, 2021

ElDeveloper Mar 17, 2021

ElDeveloper Mar 17, 2021

ElDeveloper Mar 17, 2021

ElDeveloper Mar 17, 2021

ElDeveloper Mar 17, 2021

kwcantrell Mar 23, 2021

ElDeveloper Mar 24, 2021

kwcantrell commented Mar 18, 2021

gwarmstrong Mar 18, 2021

gwarmstrong commented Mar 18, 2021

ElDeveloper commented Mar 18, 2021 via email

emperor-helper commented Mar 19, 2021

ElDeveloper left a comment

ElDeveloper commented Mar 25, 2021

Tree controller #492

Tree controller #492

Conversation

kwcantrell commented Mar 17, 2021 • edited

ElDeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwcantrell commented Mar 18, 2021

Choose a reason for hiding this comment

gwarmstrong commented Mar 18, 2021

ElDeveloper commented Mar 18, 2021 via email

emperor-helper commented Mar 19, 2021

ElDeveloper left a comment

Choose a reason for hiding this comment

ElDeveloper commented Mar 25, 2021

kwcantrell commented Mar 17, 2021 •

edited