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

Toy model view #16

Merged
merged 23 commits into from
Feb 26, 2018
Merged

Toy model view #16

merged 23 commits into from
Feb 26, 2018

Conversation

kwcantrell
Copy link
Collaborator

This is resolving issue #7 and #8

Copy link
Collaborator

@mortonjt mortonjt left a comment

Choose a reason for hiding this comment

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

I think this is an awesome first pass!

Overall summary

  1. Great to see the iterative version of the coordinate calculations working! Just to need to get that refactored a little and it should be good to go!
  2. The ToyModel.py should be merged into model.py, so that we don't have two different model classes floating around
  3. The view model could be fleshed out a little bit to enable more flexibility. I'd even suggest testing this out on a small example (see here) and try to decorate tree according to the traits under metadata.tsv
  4. We'll want to have some unittests to sanity check these methods.

Great work!

from skbio import TreeNode
import pandas as pd
import numpy as np
class ToyModel(TreeNode):
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to reduce code redundancy, I suggest

  1. Move this into model.py
  2. Rename this to Model, rather than ToyModel.

#Needs to be cleaned up
#Note: This function assumes a node can have n-children
# I believe we are only going to use bifurcating trees
# which means this code can be cleaned up quite a bit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I'd suggest cleaning this up. I think the preorder method will help simply this, as it can emulate a stack to perform depth-first search.

pass


class Model(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move all of the class methods within ToyModel into here.

self.mask = np.array()

# Coordinate manipulation
def layout(self, layout_type):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can move the contents within the coords method here.

import numpy as np
import matplotlib.pyplot as plt

def plot(node_metadata, edge_metadata):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have this method accept an axes object? Preferably something similar to the standards in seaborn (see the boxplot example here)

The advantage of doing this is that we can enable multiple subplots -- we can embed trees alongside heatmaps, boxplots, scatter plots, and many other useful visualizations.

Copy link
Collaborator

@mortonjt mortonjt Jan 19, 2018

Choose a reason for hiding this comment

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

Along the same lines, I'd suggest looking at the boxplot API to see how colors are specified.

For example, it would be extremely useful if the following API were available

def plot(node_metadata, edge_metadata, 
              node_color=None, node_size=None, 
              node_alpha=None, edge_color=None, 
              edge_width=None, edge_alpha=None):
    """
    Parameters
    -----------
    node_metadata : pd.DataFrame
        Contains all of the species attributes.
        Every row corresponds to a unique species
        and every column corresponds to an attribute.
        Metadata may also contain ancestors.
    edge_metadata : pd.DataFrame
        Contains all of the edge attributes.
        Every row corresponds to a unique edge
        and every column corresponds to an attribute.
    node_color : str
        Name of column in `node_metadata` to plot node colors.  If None, all nodes will be colored black.
    node_size : str
        Name of column in `node_metadata` to resize the nodes.  If None, all nodes will be 10.
    node_alpha : str
        Name of column in `node_metadata` to specify transparency of nodes.  If None, the nodes won't be transparent.
    edge_color : str
        Name of column in `edge_metadata` to plot edge colors.  If None, all edges will be colored black.
    edge_size : str
        Name of column in `edge_metadata` to resize the nodes.  If None, all edge will have a width of 1.
    edge_alpha : str
        Name of column in `edge_metadata` to specify transparency of edges.  If None, all edges won't be transparent.
    ax : matplotlib.axes.Axes
         optional matplotlib axes object

    Returns
    --------
    matplotlib.axes.Axes
         Axes with fully rendered tree
    """

col_node_x = node_metadata['x']
col_node_y = node_metadata['y']
# TODO: annotation on points
plt.scatter(col_node_x,col_node_y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest swapping this out with an matplotlib Axes object. This will not only make the code more modular, but it will allow for multiple plots to coexist (plt is a global variable and should be avoided when making visualization libraries).

Contains all of the edge attributes.
Every row corresponds to a unique edge
and every column corresponds to an attribute.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend returning an axes object here

@YimengYang YimengYang merged commit f01be32 into biocore:master Feb 26, 2018
YimengYang pushed a commit to YimengYang/phyloviz that referenced this pull request Oct 26, 2019
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.

None yet

4 participants