Ultrametric option

[gwarmstrong]

@gibsramen and I threw together a method for rendering trees as ultrametric (see #279 )

Here are some screenshots of the ultrametric trees:
Rectangular

Circular

Unrooted

I am unsure why the unrooted layout does not appear to have equal root to tip distances is unclear to me and I could use some guidance here.

This method relies on the algorithm as described in comments and below:

         The lengths for intermediate nodes are effectively "stretched" until
         their deepest descendant hits the deepest level in the whole tree.

         E.g., if we are at the node represented by * in the tree below:

         |--------------------------maxDistance-------------------------|
         |--distanceAbove--|           |---distanceBelow---|
                            |-length--|                     |-remainder-|
                                                    ____
                                        ___________|
                            *__________|           |_______
          __________________|          |__
                            |
                            |___________________________________________

         then the branch will be extended so that its deepest tip has the
         same depth as the deepest tip in the whole tree,
         i.e., newLength = length + remainder
         however, below it is equivalently calculated with
         newLength = maxDistance - distanceAbove - distanceBelow

         E.g.,
         |--------------------------maxDistance-------------------------|
         |--distanceAbove--|                        |---distanceBelow---|
                            |-length--||-remainder-|
                                                                 ____
                                                     ___________|
                            *_______________________|           |_______
          __________________|                       |__
                            |
                            |___________________________________________

        Repeated in a pre-order traversal, this will result in an ultrametric tree

Questions for reviewers

• Do you know why the unrooted layout renders as it does?
• I did not spend a ton of time thinking out how this should be exposed to the user. Currently it is a checkbox under the layout page, and if it is check the "Ignore branch lengths" checkbox is ignored. How do you think we should code the interaction between these two functionalities?

[emperor-helper]

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

[fedarko]

This looks awesome! Will go through things in depth tomorrow.

I did not spend a ton of time thinking out how this should be exposed to the user. Currently it is a checkbox under the layout page, and if it is check the "Ignore branch lengths" checkbox is ignored. How do you think we should code the interaction between these two functionalities?

If these options are mutually exclusive, I think using radio buttons for this might make sense for now. Alternately, we could set up something where clicking the "Make tree ultrametric?" checkbox (analogous to the "Draw barplots?" checkbox) hides the "ignore lengths" stuff and opens up a <div> with options for which ultrametric method to use. (But since there's just one ultrametric method supported -- for now at least -- I think just using radio buttons would be the easiest.)

[kwcantrell]

Thanks @gwarmstrong and @gibsramen, this is really cool.

Do you know why the unrooted layout renders as it does?

I'll be able to explain this better during our meeting today. Basically, the above algorithm works with circular/rectangular because branch lengths will always travel from a common point/direction. For example, in rectangular layout, branch lengths will always stretch the tree in the +x direction. Similarly in circular layout, all branch lengths will project out from a common point. This "standardizes" branch lengths. For example, say we have two sets of branches a->b and c->d, 'a' has a length of 1, 'b' has a length of 5, and both 'c' and 'd' have a length of 3. This means 'b' and 'd' will have a "total aggregate" length of 6. Thus, in rectangular layout, the x coordinate of 'b' and 'd' will be 6 and in circular layout, the l2 distance of 'b' and 'd' from the root will be equivalent. Thus, by setting the "total aggregate" length of all tips to be the same will make the tree appear ultrametric. However, in unrooted layout, you can essentially think the direction a node travels is "random". Basically, just because two nodes have the same "total aggregate" length, not necessarily mean they will be the same distance from the root (i.e. the l2 distance of 'b' and 'd' from the root will not be equivalent).

tldr in unrooted making the "total aggregate" length of all tips equivalent does not guarantee they will be the same distance from the root.

[gwarmstrong]

tldr in unrooted making the "total aggregate" length of all tips equivalent does not guarantee they will be the same distance from the root.

Ah okay, I think my confusion can be explained away as a misunderstanding of where the root was. So to clarify, the root of the tree in the unrooted layout is probably somewhere around here?

[kwcantrell]

Ah okay, I think my confusion can be explained away as a misunderstanding of where the root was. So to clarify, the root of the tree in the unrooted layout is probably somewhere around here?

Correct, so the "aggregate" branch lengths of the tips are the same however the l2 distances are different.

[fedarko]

Thanks @gwarmstrong and @gibsramen! This looks solid (and the algorithm seems to me like a really elegant way of doing this). I have some suggestions, but most of them are cosmetic things -- this is going to be super useful.

[fedarko]

Yeah, that's fair. My take on this is that I think figuring out lengths junk should be the job of the stuff in LayoutsUtils, just so we can avoid having logic in the main Empress class as much as we can (since it's already probs too big for its own good, and also to make testing easier).

[gwarmstrong]

I think that passing a function for the lengths is nice because it opens the door for us to do some pretty cool stuff later, without having to make modifications to the LayoutsUtil module that would make those functions signatures more unmanageable.

E.g., say we wanted a feature where you could adjust branch lengths on the fly, you could define some function like

var userSetLengths = // some Object containing lengths that the user set explicitly
lengthGetter = function(i) {
    if (i in userSetLengths) {
        return userSetLengths[i];
    } else {
        return this._tree.length(i);
}

And it shouldn't really be LayoutsUtil's job to support this.

[gwarmstrong]

What if we moved the logic from above into an Object of predefined length getters that lives in LayoutsUtil but leave the Layout functions extensible by functions.

E.g., this block would look something more like

lengthGetter = LayoutsUtil.lengthGetters[branchMethod];
if (this._currentLayout === "Rectangular") {
    data = LayoutsUtil.rectangularLayout(
        ...,
        lengthGetter
    )
}

[gwarmstrong]

Let me know if this commit 8020902 resolve this.

[fedarko]

Whoops, sorry for taking so long to get back to you on this. Yeah I can definitely see the utility of defining this behavior with functions -- I think mainly I was just hesitant to add more stuff to Empress. I like 8020902's solution to this a lot; I think having this as a function that lives in LayoutsUtil (or at least somewhere that is outside of Empress and outside of each individual layout function in LayoutsUtil) gets us the best of both worlds.

I think this is basically resolved, IMO, although I do think that now that we're making this change we should probably bite the bullet and remove the ignoreLengths parameters. If it's possible it'd be nice to do that in this PR (just to avoid confusion with having these redundant arguments still existing in the codebase -- I don't think deprecation is necessary since AFAIK no one is really out here relying on Empress' APIs), but if you think it'd be too much of a pain I'm cool to open a new issue for it.

[gibsramen]

Profiled the time to ultrametrify (officially coining this term) the EMP tree using performance.now(). Looks like, on average, the computation time of the ultrametric lengths is around 500+-50ms. This was for both rectangular & circular layouts.

Sandwiched this block of code per @gwarmstrong with time calls and took the difference.

[fedarko]

Some requested changes; overall this is looking great. Exciting stuff :D 🌳 🌲 🎋

[fedarko]

Whoops, sorry for taking so long to get back to you on this. Yeah I can definitely see the utility of defining this behavior with functions -- I think mainly I was just hesitant to add more stuff to Empress. I like 8020902's solution to this a lot; I think having this as a function that lives in LayoutsUtil (or at least somewhere that is outside of Empress and outside of each individual layout function in LayoutsUtil) gets us the best of both worlds.

I think this is basically resolved, IMO, although I do think that now that we're making this change we should probably bite the bullet and remove the ignoreLengths parameters. If it's possible it'd be nice to do that in this PR (just to avoid confusion with having these redundant arguments still existing in the codebase -- I don't think deprecation is necessary since AFAIK no one is really out here relying on Empress' APIs), but if you think it'd be too much of a pain I'm cool to open a new issue for it.

[fedarko]

Actually, uh, hm. So like I can see in the demo that clade collapsing does definitely work with ultrametric trees but... uh, I am not 100% sure why it's working??? I remember we had to jump through a few hoops to get ignore lengths to play nicely with clade collapsing.

Maybe part of it is that, like... since all of the tips wind up at the same "displacement", maybe whatever we set as the "deepest" node of a clade using the length info (here-ish) doesn't matter? Weird.

In any case it would probably be good to dig into this a bit at this morning's meeting. At the very least we should document why things are working for the sake of future us :P

[fedarko]

Two very, very small comments (one for dev documentation and one for user documentation). Basically good to merge as is.

[fedarko]

In the interest of getting a release out today, @ElDeveloper and I decided to just make the two small changes and merge this in. Thanks so much @gwarmstrong and @gibsramen for your work on this!!! 📏 🌲

[gwarmstrong]

Thanks @fedarko !!

[ElDeveloper]

📏 🎄