Shear UI #505

kwcantrell · 2021-03-25T20:09:45Z

I still need to finish documenting and add a couple of tests but in the meantime if @ElDeveloper and @fedarko can check the .qzv for UI bugs (i.e. test different combinations of coloring/shearing) to see if you can find errors that I did not count for that would be great! This PR branches from tree-controller so #492 needs to be merged in before this PR.

@ElDeveloper

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

…-controller

emperor-helper · 2021-03-25T20:19:40Z

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, looks GREAT and it is useful. Here's a few high-level observations:

I noticed some oddities with the barplots when you filter down to very few tips in the tree:

Notice the selection of only Bacteria and only Thermi.

Seems like it's also an issue with the rectangular layout:

Do you think we should include a warning message that reminds users that hiding elements in the tree can potentially be misleading?
Do you also think we should also include a warning about the ordination not being affected by any of the visibility changes?
Can we have an easy way to deselect all from a layer?
The shearing panel shows up in the UI even when there's no feature metadata, see for example plain.qzv.

empress/support_files/templates/side-panel.html

ElDeveloper · 2021-03-29T17:36:07Z

empress/support_files/css/empress.css

@@ -585,6 +585,13 @@ p.side-header button:hover,
    white-space: nowrap;
 }

+.shear-layer-legend div {


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?

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.

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

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?

@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.

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).

empress/support_files/js/empress.js

ElDeveloper · 2021-03-29T18:15:37Z

empress/support_files/js/empress.js

+            keepNames.push(name);
+        }
+
+        this._tree.shear(new Set(keepNames));


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.

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?

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.

ElDeveloper · 2021-03-29T18:17:03Z

empress/support_files/js/legend.js

     */
    Legend.prototype.addCategoricalKey = function (name, info) {
-        if (_.isEmpty(info)) {


Why is the error check being removed?

This is removed to handle cases where all tips are removed.

For example:

clicking on k__Archaea leads to:

empress/support_files/js/shearer.js

ElDeveloper · 2021-03-29T18:19:34Z

empress/support_files/js/shearer.js

@@ -0,0 +1,208 @@
+define(["underscore", "util", "TreeController"], function (


Great to see this module is using MVC. Even though objects are not exported outside of this closure, I would still suggest that you namespace them, for example: ShearingLayer, ShearModel....

Thanks @kwcantrell

kwcantrell · 2021-03-29T19:04:56Z

Thanks @ElDeveloper! Those oddities you pointed out are going to be bugs. When implementing the shear I did not really test out the sample metadata for the barplots as I thought it would behave like feature metadata but I guess that is not the case. I'll need to look the code to figure out what is causing it.

ElDeveloper · 2021-03-29T20:56:58Z

Thanks for looking into it!

fedarko

This is a really neat change, thanks! It looks like there are a decent amount of things that could stand to be improved before we throw this at users; let me know if you have any questions.

fedarko · 2021-03-30T00:07:14Z

empress/support_files/js/empress.js

+        this.getLayoutInfo();
+
+        // 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();
+            }
+        }


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?

fedarko · 2021-03-30T00:08:27Z

empress/support_files/js/empress.js

+        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);
+
+        this.getLayoutInfo();


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?

fedarko · 2021-03-30T00:11:02Z

tests/test-legend.js

-            throws(function () {
-                legend.addCategoricalKey("oops", {});
-            }, /Can't create a categorical legend when there are no categories in the info/);
-        });


Ditto with @ElDeveloper's comment above in legend.js -- I don't see why this should be removed. Does this PR involve the use of "empty" feature metadata categories somehow?

It is possible to remove all feature metadata categories in cases where all tips where removed from the tree or the remaining tips do not have metadata associated with them.

Actually I think if nodes dont have metadata they get assigned a "N.A." value. But the reason I removed it was the use case where all tips where sheared from the tree.

empress/support_files/templates/side-panel.html

empress/support_files/js/shearer.js

empress/support_files/js/empress.js

fedarko · 2021-03-30T02:21:04Z

empress/support_files/js/empress.js

+    /**
+     *
+     */
+    Empress.prototype.getUniqueSampleMetadataInfo = function (cat) {


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).

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.

@ElDeveloper

added some suggestions by @ElDeveloper and @fedarko Co-authored-by: Marcus Fedarko <marcus.fedarko@gmail.com> Co-authored-by: Yoshiki Vázquez Baeza <yoshiki@ucsd.edu>

… shear-ui

kwcantrell · 2021-06-10T21:59:12Z

@fedarko I am glad you caught the visual bug (4). I had changed the way empress.resetTree() worked so that it would only call getTreeCoords if the tree was collapsed (to save a small amount of time). I guess the way we cache the collapsed clades it wound up causing some odd visual bugs.

I also registered a shear observer for the emperor callbacks so that they get updated when a user shears the tree. The observers will also be unregistered after the 4 second timer expires.

w.r.t. the animations, its get a bit hairy to properly deal with the shear functionality during an animation so instead I added a warning message to the animation panel.

fedarko

A few updates based on the changes made. Sorry to be a pain and thanks so much Kalen!

empress/support_files/js/emperor-callbacks.js

fedarko · 2021-06-10T23:55:59Z

empress/support_files/templates/side-panel.html

+  <p class="side-panel-notes">
+    <b>Warning:</b> Do not shear the tree while an animation is running. Please
+    stop and restart if the tree was sheared during an animation.
+  </p>


Thanks for adding this -- looks good!

If we are going to completely say that the user shouldn't do shearing during an animation, I think it might make sense to just explicitly prevent this -- I assume this warning would also apply to synchronized animations that Emperor starts?

Maybe we could have the Animator class update the Empress class somehow (so that the Empress class always has knowledge of if an animation is ongoing), and then ShearModel could use that knowledge to only apply shearing changes when an animation isn't ongoing. Or, IDK, you would know best how to do that :)

...We don't have to do this before merging, tho -- I recognize that this PR has been waiting for a while, so I'm ok if we want to get this in and then take care of preventing this case later.

kwcantrell · 2021-06-11T06:59:21Z

@fedarko I went ahead and added a bunch of code to disable sample/feature metadata coloring, barplots, and shear tree while an animation is active. Since I added some new code, I am sure you can continue to be "pain" 😆 (please continue to be a "pain" as that will ultimately lead to a better feature). This is how the UI looks while an animation is active:

Additionally, there was another case where the user started the animation using the emperor interface. In that case, I also disable the Animation tab within empress.

fedarko

Thank you so much, @kwcantrell! I'm really excited by these changes -- this is very comprehensive. I think this PR will actually also close #195, an issue that it turns out we opened exactly one year ago (!!). So that's neat.

All I have at this point are minor suggestions. One more round and I think we can merge this in!!!

It isn't necessary by any means, but I also think we can reduce the size of the warning messages for the disabled tabs a bit -- we can do this by adding another flag variable, is_empire_plot, that is passed from core.py to empress-template.html to the EnableDisableSidePanelTab objects. This flag variable will control whether or not the note about Emperor animations will be shown in e.g. the sample/feature metadata coloring tabs while they're disabled (so, if there isn't an Empire plot, then we can just show a smaller warning). If you're ok with it, I propose the following changes to three files within the code:

3-modified-files.zip

I've tested this on a checked-out version of this branch, and I think this should work. The differences between these three files and the repo can be assessed by copying them in and using git status (note that my other suggestions in this review should be applied on top of these changed files).

Thanks again, and let me know if I can be of further help.

empress/support_files/templates/empress-template.html

empress/support_files/js/enable-disable-side-panel-tab.js

empress/support_files/templates/empress-template.html

empress/support_files/js/enable-disable-side-panel-tab.js

empress/support_files/js/enable-disable-tab.js

empress/support_files/templates/empress-template.html

empress/support_files/js/enable-disable-animation-tab.js

added suggestions Co-authored-by: Marcus Fedarko <marcus.fedarko@gmail.com>

…5-20210716

Reason for removing \t characters is that these were the only files in the Python / JS codebase that used literal tab characters (instead of ordinary spaces).

The other tabs don't use bold for "This tab" and "To re-enable this tab", so we don't use it here either.

kwcantrell · 2021-07-17T02:56:20Z

Thanks for the many reviews @fedarko! 😆 This feature went through a lot of iterations and I think the final product is way more polished as a result.

fedarko · 2021-07-17T02:58:39Z

Thanks so much @kwcantrell (and @ElDeveloper)! It's really great to finally have this in :)

At long last, I guess we can push out a release early next week? 👀 👀 👀

kwcantrell · 2021-07-17T03:16:39Z

Its definitely time for a release. Although, if we can merge #484 , #521 (updated to include #484) and the new adjust color alpha feature first, then I think we will truly have the "next gen" release! 😲

I believe #484 and #521 just have a few issues left to address and the adjust color alpha is ready for review (I was just waiting for #505 to be merge before I opened the PR as it abstracts/builds on #505).

However, I am totally fine with cutting a release next week!

fedarko · 2021-07-17T03:35:37Z

The idea of Next-Generation EMPress ™️ certainly sounds enticing ;) However, I'd vote we cut a release ASAP, just because the review process can sometimes take a while (... as I guess was demonstrated by me on this PR 😅 ), and it's been some time since our last release.

We could always cut one release next week, and then cut another release soon afterwards with #484 / #521 / alpha adjustment / etc. I have a few features in prep that I hope to get ready soon as well (#322, #371, #504).

kwcantrell · 2021-07-17T03:37:24Z

Okay, that sounds like a plan then! Lets to cut a release next week!

kwcantrell and others added 18 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

style fix

a408326

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

addressed comments

5ca5809

removed shear

89a4326

start ui

654df69

finished basic shear ui

d6aa515

fixed test issue

de01d93

removed old comments

fcb6b5b

merged master

55f6f7e

fedarko self-requested a review March 26, 2021 01:42

ElDeveloper requested changes Mar 29, 2021

View reviewed changes

fedarko mentioned this pull request Mar 30, 2021

Use smoother curves to draw circular layout barplots for trees with only a few tips #508

Open

fedarko requested changes Mar 30, 2021

View reviewed changes

kwcantrell and others added 4 commits March 30, 2021 13:27

Apply suggestions from code review

b3acbd1

added some suggestions by @ElDeveloper and @fedarko Co-authored-by: Marcus Fedarko <marcus.fedarko@gmail.com> Co-authored-by: Yoshiki Vázquez Baeza <yoshiki@ucsd.edu>

started addressing PR suggestions

263dd6a

Merge branch 'shear-ui' of https://github.com/kwcantrell/empress into…

5bb8729

… shear-ui

fixed style

68d0d63

fedarko reviewed Jun 10, 2021

View reviewed changes

kwcantrell added 3 commits June 10, 2021 17:29

merged with master

aedb860

disabled tabs during animation

90571be

fixed tests/lint issues

149ecb5

fedarko reviewed Jun 22, 2021

View reviewed changes

kwcantrell and others added 4 commits June 29, 2021 13:19

Apply suggestions from code review

288a140

added suggestions Co-authored-by: Marcus Fedarko <marcus.fedarko@gmail.com>

modified message

7faa70a

addressed comments

cb085ba

fixed python test

8af9b93

fedarko self-requested a review July 8, 2021 02:23

fedarko added 10 commits July 16, 2021 17:46

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

c0fbd57

…5-20210716

Remove extra " in side panel disabled message

0505705

rm another extra "

924b938

space after "

0bae2ce

fix problem i introduced in the last commit ._.

953abd8

MNT: RM \t chars, make e-d-*.js docs consistent

8767132

Reason for removing \t characters is that these were the only files in the Python / JS codebase that used literal tab characters (instead of ordinary spaces).

DOC: Adjust tab-disabled prefix using isEmpirePlot

01d7022

DOC: Make uses of bold fonts consistent

c57a5d2

The other tabs don't use bold for "This tab" and "To re-enable this tab", so we don't use it here either.

STY: prettify

00b7dee

Cleaner toast msg for no-FM-b/c-shearing case

b664bfd

fedarko mentioned this pull request Jul 17, 2021

Need to un-hide "Update" buttons when enabling sample / feature metadata coloring tabs after finishing an animation #532

Open

fedarko merged commit d5193ab into biocore:master Jul 17, 2021

fedarko mentioned this pull request Jul 17, 2021

Interaction bugs between animation and sample / feature metadata coloring #195

Closed

		@@ -0,0 +1,208 @@
		define(["underscore", "util", "TreeController"], function (

Shear UI #505

Shear UI #505

Conversation

kwcantrell commented Mar 25, 2021

emperor-helper commented Mar 25, 2021

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

fedarko Apr 30, 2021 • edited Loading

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 29, 2021

ElDeveloper commented Mar 29, 2021

fedarko 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

kwcantrell commented Jun 10, 2021

fedarko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwcantrell commented Jun 11, 2021

fedarko left a comment

Choose a reason for hiding this comment

kwcantrell commented Jul 17, 2021

fedarko commented Jul 17, 2021

kwcantrell commented Jul 17, 2021

fedarko commented Jul 17, 2021

kwcantrell commented Jul 17, 2021

fedarko Apr 30, 2021 •

edited

Loading