Animation #146

kwcantrell · 2020-03-28T18:08:51Z

No description provided.

Bp tree

…rawer

…is-test

…nto side-panel

ElDeveloper

Looks good, thanks so much Kalen. I've made a few comments but they are all fairly minor.

empress/_plot.py

ElDeveloper · 2020-03-28T22:34:54Z

empress/_plot.py

@@ -78,6 +78,22 @@ def plot(output_dir: str,
        .to_dataframe().filter(feature_table.index, axis=0) \
        .to_dict(orient='index')

+    # TODO: Empress is currently sorting all metadata as strings. This is


I think sample metadata is in general fine loaded as a string. Have you encountered any issues?

empress/support_files/css/empress.css

ElDeveloper · 2020-03-28T22:37:51Z

empress/support_files/js/drawer.js

        this.cam = cam;
        this.VERTEX_SIZE = 5;
+
+        // sets empress to night mode
+        // TODO: make this a customizable parameter


I think this comment can be removed based on @fedarko's issue here #143

Suggested change

// TODO: make this a customizable parameter

// TODO: make this a customizable parameter

empress/support_files/js/animation-panel-handler.js

empress/support_files/js/animator.js

fedarko

This looks great @kwcantrell! I suggested a few changes -- some of these might take a nontrivial amount of work, so if you'd prefer to defer some to new issues on GitHub that's totally fine. (Or if you disagree with anything, just let me know :)

I'll try to get that big insertion tree animated through here later today and will let you all know how that goes.

empress/_plot.py

empress/support_files/templates/animate-panel.html

fedarko · 2020-03-30T20:10:56Z

empress/support_files/js/animation-panel-handler.js

+
+        // dont show next button on last frame
+        if (this.animator.onLastFrame()) {
+            this.nextFrameBtn.classList.add("hidden");


A slight problem with this approach is that the buttons are positioned such that, when the Next Frame button is hidden, the Previous Frame button is shifted over to where this button previously was:

Second-from-last frame in an animation

Last frame in an animation

This means that, for people who are manually clicking through the animation, they'll likely accidentally press Previous Frame when they get to the final frame. Not a big deal, but an annoyance nonetheless.

IMO it would be better to absolutely position these buttons, to attempt to stave off this problem.

Nice catch! I'll update the PR to disable the prev/next button when on the first/last frame instead of hiding them.

fedarko · 2020-03-30T20:15:46Z

empress/support_files/js/animation-panel-handler.js

+        this.startBtn.classList.add("hidden");
+        this.resumeBtn.classList.add("hidden");
+        this.prevFrameBtn.classList.add("hidden");
+        this.nextFrameBtn.classList.add("hidden");


In my opinion at least :), it would be nice to have the prev/next-frame buttons present from the start of the animation -- it wasn't clear to me initially that these buttons were even available until I pressed Pause. For users who want to step through the animations at their own pace (either to go slower or faster), I suspect these buttons will be preferable.

@fedarko thanks for the suggestion! My goal with the GUI is to make things as intuitive and easy to use. One way I try to accomplish this is by only presenting options to the user that can be applied at that moment. Since the prev/next features can only be used when the animation is paused, I decided to hide those options unless the user pauses the animation.

Instead of hiding those buttons, I can disable them while the animation is running and enable them when the animation is paused. Also, I can start the animation is the pause state instead of the run state so users who wanted to manually step through the animation could do so without having to first pause the animation first.

@ElDeveloper how does Emperor handle this?

In emperor we disable the buttons and still leave them visible (instead of hiding them).

fedarko · 2020-03-30T20:26:29Z

empress/support_files/templates/animate-panel.html

+    <label for="animate-trajectory">Trajectory</label>
+    <label class="select-container">
+      <select id="animate-trajectory"></select>
+    </label>


Not really needed for this PR, but I'm just going to tag #138 here -- this would be a great place to add tooltips in the future explaining what exactly Gradient and Trajectory mean, for new users and/or for folks who haven't used the animation functionality in Emperor before.

Great suggestion!

Great suggestion @fedarko, I suggest doing that now using the title= attribute.

fedarko · 2020-03-30T20:29:23Z

empress/support_files/templates/side-panel.html

@@ -31,7 +31,7 @@
      </p>
      <p>
        <label for="sample-line-width">Line Width</label>
-        <input id="sample-line-width" type="number" value="1">
+        <input id="sample-line-width" type="number" value="4">


Not clear why the default is switched to 4 here? As #144 describes, line widths this large can start to look pretty jumbled when you inspect details of the tree. I would suggest keeping 1 as the default (it's also easier to see colored lines on a white background, IMO; another reason to keep light mode as the default)

This ties into the comment I posted on #144. The main issue comes from how line widths are calculate in empress. To create new lines, a rectangle is created (in tree space) whose width is 2*line_width. So, a line width of 4 can appear larger/smaller depending on how much space the tree spans.

I set the default to 4 before #144 came up because the tree I, when testing the animation feature, a line width of 4 looked the best. I guess I forgot to switch it back.

Understood! That makes sense to me. We'll probably have to reevaluate stuff for #144, but until then I guess the defaults don't matter too much.

fedarko · 2020-03-30T20:33:34Z

empress/support_files/js/animator.js

+        //draw tree
+        this.empress.resetTree();
+        this.empress._colorTree(obs, this.cm);
+        this.empress.thickenSameSampleLines(this.lWidth);


This behavior differs from the normal sample coloring behavior -- in SidePanel.prototype._updateSample(), the call made is

if (lWidth !== 1) { this.empress.thickenSameSampleLines(lWidth - 1); }

This inconsistency means that the same line width will result in differently-drawn rectangles between the sample coloring stuff and between the animation stuff:

Animation drawn with line width = 1:

Normal sample coloring, with line width = 1:

(Notice how the top screenshot's lines are drawn as being notably thicker than the bottom screenshot's.)

I would strongly suggest making this consistent (one way or another). That being said, we'll probably come back to this stuff when we get around to #144, so not a huge issue.

Good catch! SidePanel (which probably should be renamed to SampleColorer or something like) should not be responsible for adjusting the line width as that operation should be handled by the thickenSameSampleLines (which also should be renamed since it is thickens all colored branches not same sample lines).

I'll update #144 to include this.

empress/support_files/js/biom-table.js

fedarko · 2020-03-30T21:05:25Z

empress/support_files/js/legend.js

+            // TODO: add total percentage of tree colored by each category
+            //       old method was not correct.


It's still not really clear how the previous method was incorrect from looking at #148. Not urgent or anything, but describing the bug somewhere would be nice (if nothing else, so that someone unfamiliar with it like me doesn't accidentally make the same mistake again :P)

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Co-Authored-By: Marcus Fedarko <[email protected]> Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

ElDeveloper

All looks good to me, except for the tooltips. If those are added and @fedarko's comments are all addressed, I think this is good to be merged.

ElDeveloper · 2020-03-31T23:25:44Z

empress/support_files/js/biom-table.js

+        }
+
+        // convert result to array and sort
+        values = [...values];


The number of samples can be up to hundreds of thousands. Not sure what method might be better these days, but I think Array.from might be better.

ElDeveloper · 2020-03-31T23:26:29Z

empress/support_files/js/drawer.js

+
+        // sets empress to night mode
+        // TODO: make this a customizable parameter
+        this.CLR_COL = 0;


Let's keep the light background until this can be user-modifiable.

empress/support_files/js/empress.js

empress/support_files/templates/animate-panel.html

ElDeveloper · 2020-03-31T23:28:55Z

empress/support_files/templates/animate-panel.html

+    <label for="animate-trajectory">Trajectory</label>
+    <label class="select-container">
+      <select id="animate-trajectory"></select>
+    </label>


Great suggestion @fedarko, I suggest doing that now using the title= attribute.

empress/support_files/js/drawer.js

ElDeveloper · 2020-04-06T18:54:37Z

Thanks everyone!

🎆 🚀 🎥

kwcantrell and others added 30 commits July 29, 2019 12:56

added camera/tests

d992b88

added tests

c6cc202

checkpoint

3714011

naive implementation of bp tree

78f2383

added travis

476b555

removed travis

d930e75

added preorder

790129a

Merge pull request #16 from kwcantrell/bp-tree

3c9b2fa

Bp tree

added basic drawer

9044382

added space

5c686cc

addressed PR comments

8e4ac07

addressed PR comment

bcabaab

Merge branch 'master' of https://github.com/kwcantrell/empress into d…

4b26046

…rawer

added hello-world test

fbf8ece

moved package.json

6db1d0c

changed travis

7e68666

added dep

029f7b3

update

e53a9a6

added yarn

1f38def

added dev

ffdaaec

npm

41314c3

npm

ef59362

nodeJS 10

a04ae25

removed node_modules

81157ba

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

40fb511

…is-test

intial side panel

08622f1

fixed tests

9e574b9

fixed travis test

88df7b4

added cache for open-close

15d7407

Merge branch 'bp-oc-cache' of https://github.com/kwcantrell/empress i…

e31bbd4

…nto side-panel

ElDeveloper requested review from fedarko and ElDeveloper March 28, 2020 21:24

ElDeveloper approved these changes Mar 29, 2020

View reviewed changes

fedarko requested changes Mar 30, 2020

View reviewed changes

kwcantrell and others added 20 commits March 30, 2020 15:26

Update empress/_plot.py

2061e91

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/css/empress.css

6ce0cc9

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/biom-table.js

8403e81

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/biom-table.js

8217863

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

f274b1e

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/biom-table.js

b2ded8c

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/empress.js

f0a0f95

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

4d2b051

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

02fc054

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

ee93619

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

b2d7929

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

bc52acc

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

15c0499

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

2795510

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

23f8095

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animation-panel-handler.js

19fdbc5

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Apply suggestions from code review

4ee966f

Co-Authored-By: Marcus Fedarko <[email protected]> Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

Update empress/support_files/js/animator.js

9a92ae1

Co-Authored-By: Yoshiki Vázquez Baeza <[email protected]>

biocore#146

e478628

Merge branch 'master' of https://github.com/kwcantrell/empress

ef7daa0

ElDeveloper approved these changes Mar 31, 2020

View reviewed changes

back to light mode

4ff2664

fedarko reviewed Apr 6, 2020

View reviewed changes

empress/support_files/js/drawer.js Outdated Show resolved Hide resolved

Update empress/support_files/js/drawer.js

262374f

fedarko merged commit 3417f59 into biocore:master Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Animation #146

Animation #146

kwcantrell commented Mar 28, 2020

ElDeveloper left a comment

ElDeveloper Mar 28, 2020

ElDeveloper Mar 28, 2020

fedarko left a comment

fedarko Mar 30, 2020

kwcantrell Mar 31, 2020

fedarko Mar 30, 2020

kwcantrell Mar 31, 2020 •

edited

Loading

ElDeveloper Mar 31, 2020

fedarko Mar 30, 2020

kwcantrell Mar 31, 2020

ElDeveloper Mar 31, 2020

fedarko Mar 30, 2020

kwcantrell Mar 31, 2020

fedarko Mar 31, 2020

fedarko Mar 30, 2020

kwcantrell Mar 31, 2020

fedarko Mar 30, 2020

ElDeveloper left a comment

ElDeveloper Mar 31, 2020

ElDeveloper Mar 31, 2020

ElDeveloper Mar 31, 2020

ElDeveloper commented Apr 6, 2020

	// TODO: make this a customizable parameter
	// TODO: make this a customizable parameter

		// TODO: add total percentage of tree colored by each category
		// old method was not correct.

Animation #146

Animation #146

Conversation

kwcantrell commented Mar 28, 2020

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

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

kwcantrell Mar 31, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ElDeveloper commented Apr 6, 2020

kwcantrell Mar 31, 2020 •

edited

Loading