Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Radialplot #112

Merged
merged 16 commits into from
Mar 1, 2017
Merged

WIP: Radialplot #112

merged 16 commits into from
Mar 1, 2017

Conversation

mortonjt
Copy link
Collaborator

@mortonjt mortonjt commented Feb 11, 2017

Depends on #110

This creates unrooted radial tree visualizations using bokeh.

Right now, this creates an interactive tree, with zoom, reset and name highlighting capabilities.

This is attempting to follow the seaborn paradigm, using the tree and the internal data structure for plotting instead of a pandas dataframe.

In the future, we'll need to create methods that'll make it easy to perform bulk operations to append values to scikit-bio trees. Maybe some methods that take in pd.DataFrame and decorates all of the nodes and edges in the tree with those attributes. That is a future PR.

There are a couple of concerns that will need to be addressed here.

  • Unittests. Not sure how to do so with bokeh just yet.
  • The interactive component could provide a substantial amount of customizability, but the current setup limits that, due to the explosion of parameters required to enable that. Another option is to not include the interactive component, and let the user configure it themselves.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 97.73% when pulling 01e4f59 on mortonjt:radialplot into 18fc891 on biocore:master.

Copy link
Collaborator

@qiyunzhu qiyunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jamie always does a great job!


else:
self.height = self.length
self.leafcount = self.edgecount = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know this double-"=" works!

if children:
for c in children:
c.update_geometry(use_lengths, self.depth)
self.height = max([c.height for c in children]) + self.length
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are height and length? Are they describing tree branches? In FigTree height and length have some different and statistical meanings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

height is the total height of the subtree. length is the branch length connecting to that subtree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the docstring for it and specify it in the attributes

from bokeh.models.glyphs import Circle, Segment
from bokeh.models import ColumnDataSource, DataRange1d, Plot
from bokeh.models import HoverTool, BoxZoomTool, ResetTool
except ImportError:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering in this situation, ImportError and/or ImportWarning will be raised eventually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to avoid my dependencies blowing up - so all of the visualization code that I'm using will be based on optional dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I also wonder how to set up Travis to test both scenarios. Can one install / not install a package simultaneously in .travis.yaml?

node_hue : str
Name of variable in `tree` to color nodes.
node_size : str
Name of variable in `tree` that species the radius of nodes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: species => specifies

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

----------
tree : instance of skbio.TreeNode
Input tree for plotting.
node_hue : str
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the usage of term "hue" here isn't right. You actually passed the entire RGB value to hue. But to be strict hue is a single number that should be used with saturation and brightness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced hue with color. What are your thoughts?

nodes = t.coords(figsize[0], figsize[1])

# fill in all of the node attributes
default_node_hue = '#D3D3D3'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider putting a comment here what color (in English) does '#D3D3D3' mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

default_node_hue)
for n in t.levelorder(include_self=True)})

default_node_size = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to zip these two iterations into one? (I don't know I am just asking)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about something like as follows

    def _retreive(tree, x, default):
        return pd.Series({n.name: getattr(n, x, default)
                          for n in tree.levelorder()})

    nodes[node_hue] = _retreive(t, node_hue, '#D3D3D3')

@mortonjt
Copy link
Collaborator Author

mortonjt commented Feb 27, 2017 via email

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 97.718% when pulling e71a5b7 on mortonjt:radialplot into 18fc891 on biocore:master.

@mortonjt
Copy link
Collaborator Author

mortonjt commented Feb 28, 2017

@qiyunzhu all of the comments have been addressed.

Would you like to clone it and try this out yourself?
You can install this branch via

pip install git+https://github.com/mortonjt/gneiss.git@radialplot

And there's the following commands that you can run to test within a notebook.

# import stuff
from gneiss.plot import radialplot

import numpy as np
from scipy.cluster.hierarchy import ward
from skbio import TreeNode, DistanceMatrix
from bokeh.io import output_notebook, show
from gneiss.plot._dendrogram import UnrootedDendrogram
import bokeh.plotting as plt
output_notebook()

# create random tree
np.random.seed(0)
num_otus = 3  #otus 
x = np.random.rand(num_otus)
dm = DistanceMatrix.from_iterable(x, lambda x, y: np.abs(x-y))
lm = ward(dm.condensed_form())
t = TreeNode.from_linkage_matrix(lm, np.arange(len(x)).astype(np.str))
t = UnrootedDendrogram.from_tree(t)

# populate tree with colors and stuff
for i, n in enumerate(t.postorder(include_self=True)):
    if not n.is_tip():
        n.name = "y%d" % i
        n.color='#00FF00'
        n.edge_color='#FF0000'
        n.node_size=10
    else:
        n.color='#0000FF'
        n.edge_color='#00FF00'
        n.node_size=10
    n.length = np.random.rand()*3
    n.edge_width = 2

# plot and show
p = radialplot(t, node_hue='color', edge_hue='edge_color', 
               node_size='node_size', edge_width='edge_width')
show(p)

@qiyunzhu
Copy link
Collaborator

qiyunzhu commented Mar 1, 2017

I tested the code and it worked. The plot looks cool. @mortonjt Ready to ship.

@qiyunzhu qiyunzhu merged commit 92b37fa into biocore:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants