ENH: Adding vector visualization for biplots #352

mortonjt · 2015-03-02T02:45:38Z

Adding in basic vector visualization of taxa. This will ultimately allow easier

Visualization of correlations between taxa
Visualizing which taxa explains the most variability amongst the samples
Visualizing the variance of the taxa

A couple of things will need to be addressed in the future

When taxa are added, both the vectors and spheres are displayed at once. Should there be another toggle to toggle the display of the vectors? For example, the user may want to remove the spheres if they are too clustered, but would want to keep the vectors
What would be a good way to color code the taxa? They are now by default white. It may be useful to color code each vector. For example, when making biplots, I find it useful to color code the phyla

emperor-helper · 2015-03-02T02:55:36Z

The test build for this pull request can be found here: http://emperor.colorado.edu/pull_352/make_emperor/

wasade · 2015-03-02T14:58:38Z

scripts/make_emperor.py

@@ -1,5 +1,5 @@
 #!/usr/bin/env python
-# File created on 06 Jul 2012
+# File reated on 06 Jul 2012


Yup ... That was dumb ...

ElDeveloper · 2015-03-03T06:43:59Z

emperor/support_files/js/emperor.js

@@ -735,6 +736,7 @@ function colorChanged(catValue,color) {
 function colorChangedForTaxaSpheres(color){
 	for (index in g_plotTaxa){
 		g_plotTaxa[index].material.color.setHex(color)
+


Can you remove this extra white-space?

ping on this and the other extra whitespace.

Still this extra whitespace.

ElDeveloper · 2015-03-03T06:53:13Z

I like this a lot, thanks!!

When taxa are added, both the vectors and spheres are displayed at once. Should there be another toggle to toggle the display of the vectors? For example, the user may want to remove the spheres if they are too clustered, but would want to keep the vectors.

👍 and I think we would need this as part of this PR (let me know if you need help, I'm super happy to Skpye/Hangouts), otherwise people who wanted to remove the vectors (and reproduce previous plots) would not be able to do it.

What would be a good way to color code the taxa? They are now by default white. It may be useful to color code each vector. For example, when making biplots, I find it useful to color code the phyla

This is something we have discussed a lot. There's #59, #61 and #75. If you don't think what you mean would be covered by any of this issues, please feel free to add a new issue.

Overall the PR looks great, I made a few minor comments and there's a few things that would be missing:

Mention on the ChangeLog.md.
Mention about these vectors in the make_emperor.py help.
A way to turn them off.

Also modified toggleBiplotVisitbility function to condense visibility functionality into a single function

ElDeveloper · 2015-03-04T22:47:00Z

tests/test_format.py

        """Test correct formatting of the footer string"""
        # footer for a jackknifed pcoa plot without biplots
        out_string = format_emperor_html_footer_string(False, True)
-        self.assertEqual(out_string, EXPECTED_FOOTER_A)
+        self.assertItemsEqual(out_string.split('/'), EXPECTED_FOOTER_A.split('/'))


I'm not sure I understand what this change is doing, would you mind elaborating?

Its just a way to make debugging easier. Before, an entire HTML page would be dumped on the page. Using this will allow only portions of differing HTML to be displayed.

It isn't a big deal though - I can remove it.

I see, this makes sense! A follow-up question, why did you choose '/'?
'\n' makes more sense to me, but then again, I haven't yet used this in
a real-case scenario.

On (Mar-04-15|15:37), mortonjt wrote:

"""Test correct formatting of the footer string""" # footer for a jackknifed pcoa plot without biplots out_string = format_emperor_html_footer_string(False, True)

self.assertEqual(out_string, EXPECTED_FOOTER_A)

self.assertItemsEqual(out_string.split('/'), EXPECTED_FOOTER_A.split('/'))

Its just a way to make debugging easier. Before, an entire HTML page would be dumped on the page. Using this will allow only portions of differing HTML to be displayed.

It isn't a big though - I can remove it.

Reply to this email directly or view it on GitHub:
https://github.com/biocore/emperor/pull/352/files#r25826331

Just a random common letter ('/' occurs before every major html header). I think '\n' would work just as well.

I would prefer '\n' then. Also, since assertItemsEqual doesn't care
about the order of the elements, we would need to still have the
self.assertEqual check. The first would help developers identify the
lines that need to be updated and the second verifies there's a
consistently and correctly structured HTML string.

On (Mar-04-15|15:45), mortonjt wrote:

"""Test correct formatting of the footer string""" # footer for a jackknifed pcoa plot without biplots out_string = format_emperor_html_footer_string(False, True)

self.assertEqual(out_string, EXPECTED_FOOTER_A)

self.assertItemsEqual(out_string.split('/'), EXPECTED_FOOTER_A.split('/'))

Just a random common letter ('/' occurs before every major html header). I think '\n' would work just as well.

Reply to this email directly or view it on GitHub:
https://github.com/biocore/emperor/pull/352/files#r25826838

ElDeveloper · 2015-03-04T23:09:59Z

Looks great, I think we are only missing adding a description about these arrows somewhere in make_emperor.py and it should be ready to be merged!

Removing whitespace, adding semicolons and condensing toggle function Also updating the changelog and adding additional documentation in the main emperor script to ease interpretation of taxa vectors Updating test script to split by newline

ElDeveloper · 2015-03-05T01:09:11Z

@mortonjt, just a heads up, it seems like there's legitimate failures.

mortonjt · 2015-03-05T01:16:48Z

...

ElDeveloper · 2015-03-05T01:31:00Z

emperor/support_files/js/emperor.js

-function toggleBiplotVisibility(){
+/* Turn on and off the spheres representing the biplots on screen
+If the toggleArrow variable is true, then only the visibility of the taxa vectors will be altered
+Otherwise, only the visibility of the */


Can you remove this line?

While we don't have any guidelines on this, we try to keep the comments formatted in the following way:

/* Turn on and off the spheres representing the biplots on screen If the toggleArrow variable is true, then only the visibility of the taxa vectors will be altered. */

ElDeveloper · 2015-03-05T01:32:15Z

I think that's good as is, unless I'm missing something.

On (Mar-04-15|17:16), mortonjt wrote:

...

While I'm fixing those errors. Do you think that I should also modify the animate. Namely this line

Reply to this email directly or view it on GitHub:
#352 (comment)

ElDeveloper · 2015-03-05T02:11:41Z

emperor/support_files/js/emperor.js

+/* Turn on and off the spheres representing the biplots on screen
+
+If the toggleArrow variable is true, then only the visibility of the taxa vectors will be altered
+Otherwise, only the visibility of the */


This line should still be removed. And the line above should be aligned with the word Turn that's at the beginning of this block of comments.

ElDeveloper · 2015-03-05T05:58:48Z

There's just one minor (nano-comment) comment and this is ready to be merged!

wasade · 2015-03-05T15:14:04Z

emperor/support_files/js/emperor.js

-			g_mainScene.add(g_plotTaxa[index])
-		}
-	}
+    toggleArrow = typeof toggleArrow !== 'undefined' ? toggleArrow : false;


huh, well, I guess it didn't reduce the LOC, but at least the forloop is much simpler :)

ElDeveloper · 2015-03-05T19:30:38Z

Thanks @mortonjt, this is ready to be merged, though there are merge conflicts, would you mind solving them?

mortonjt · 2015-03-05T22:15:03Z

I think that should do it

ENH: Adding vector visualization for biplots

ElDeveloper · 2015-03-05T22:16:47Z

Thanks a lot @mortonjt!!

ENH: Adding vector visualization for biplots

60907a6

wasade reviewed Mar 2, 2015
View reviewed changes

mortonjt added 2 commits March 2, 2015 08:23

STY: Fixing typo in main script

562e709

DOC: Removing unecessary documentation

62c6fbc

ElDeveloper reviewed Mar 3, 2015
View reviewed changes

mortonjt added 3 commits March 3, 2015 16:41

ENH: Adding in toggle button for diplaying taxa vectors

853f0ef

Merging

d3eea03

TST: Removing whitespaces from javascript strings

19e3695

Also modified toggleBiplotVisitbility function to condense visibility functionality into a single function

ElDeveloper reviewed Mar 4, 2015
View reviewed changes

STY: Refactoring and documentation update

3cc497c

Removing whitespace, adding semicolons and condensing toggle function Also updating the changelog and adding additional documentation in the main emperor script to ease interpretation of taxa vectors Updating test script to split by newline

TST: Adding break into test case to fix test erros

6ce5132

ElDeveloper reviewed Mar 5, 2015
View reviewed changes

STY: formatting comments in toggle function

9ce1629

ElDeveloper reviewed Mar 5, 2015
View reviewed changes

STY: Adding comment and adding debugging feature in test_core

85f74de

STY: Removing whitespace

d1b388c

wasade reviewed Mar 5, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master' into biplot

3323ec1

ElDeveloper added a commit that referenced this pull request Mar 5, 2015

Merge pull request #352 from mortonjt/biplot

ccd8581

ENH: Adding vector visualization for biplots

ElDeveloper merged commit ccd8581 into biocore:master Mar 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Adding vector visualization for biplots #352

ENH: Adding vector visualization for biplots #352

mortonjt commented Mar 2, 2015

emperor-helper commented Mar 2, 2015

wasade Mar 2, 2015

mortonjt Mar 2, 2015

ElDeveloper Mar 3, 2015

ElDeveloper Mar 4, 2015

ElDeveloper Mar 5, 2015

ElDeveloper commented Mar 3, 2015

ElDeveloper Mar 4, 2015

mortonjt Mar 4, 2015

ElDeveloper Mar 4, 2015

mortonjt Mar 4, 2015

ElDeveloper Mar 4, 2015

mortonjt Mar 4, 2015

ElDeveloper commented Mar 4, 2015

ElDeveloper commented Mar 5, 2015

mortonjt commented Mar 5, 2015

ElDeveloper Mar 5, 2015

ElDeveloper commented Mar 5, 2015

ElDeveloper Mar 5, 2015

ElDeveloper commented Mar 5, 2015

wasade Mar 5, 2015

ElDeveloper commented Mar 5, 2015

mortonjt commented Mar 5, 2015

ElDeveloper commented Mar 5, 2015

ENH: Adding vector visualization for biplots #352

ENH: Adding vector visualization for biplots #352

Conversation

mortonjt commented Mar 2, 2015

emperor-helper commented Mar 2, 2015

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 commented Mar 3, 2015

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 commented Mar 4, 2015

ElDeveloper commented Mar 5, 2015

mortonjt commented Mar 5, 2015

Choose a reason for hiding this comment

ElDeveloper commented Mar 5, 2015

Choose a reason for hiding this comment

ElDeveloper commented Mar 5, 2015

Choose a reason for hiding this comment

ElDeveloper commented Mar 5, 2015

mortonjt commented Mar 5, 2015

ElDeveloper commented Mar 5, 2015