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

Util #12

Merged
merged 13 commits into from
Jul 26, 2016
Prev Previous commit
Next Next commit
STY: clean up pep8/flake8
  • Loading branch information
mortonjt committed Jul 19, 2016
commit 2ded931959028e69ef3f19e9b30ac6b9fc75c524
3 changes: 1 addition & 2 deletions gneiss/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,6 @@ def test_match_tips_intersect_tree_immutable(self):
match_tips(table, tree, intersect=True)
self.assertEqual(str(tree), u"(((a,b)f,c),d)r;\n")


def test_match_tips_mismatch(self):
# table has less columns than tree tips
table = pd.DataFrame([[0, 0, 1],
Expand Down Expand Up @@ -308,7 +307,7 @@ def test_rename_internal_nodes_names_mismatch(self):
with self.assertRaises(ValueError):
rename_internal_nodes(tree, ['r', 'abc'])

def test_rename_internal_nodes(self):
def test_rename_internal_nodes_warning(self):
tree = TreeNode.read([u"(((a,b)y2, c),d)r;"])
with self.assertWarns(Warning):
rename_internal_nodes(tree)
Expand Down
4 changes: 2 additions & 2 deletions gneiss/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ def rename_internal_nodes(tree, names=None):
"""
_tree = tree.copy()
Copy link
Member

Choose a reason for hiding this comment

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

Is the copy strictly necessary? I'm thinking if it will be okay to just do the renaming inplace - just thinking on the memory footprint of this operations as the tree size increases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The copy isn't strictly necessary. But it is certainly useful. Just added an inplace parameter here.

It'll be tricky to have this functionality in the other modules, mainly because pandas and skbio don't make filter and shear operations in place (as far as I'm aware). If you think this is necessary, we can create an issue for this, so that this PR can proceed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I follow with the second part of your comment. With the inplace parameter my comment is resolved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sweet. Then that settles it :)

On Mon, Jul 25, 2016 at 4:56 PM, Jose Navas notifications@github.com
wrote:

In gneiss/util.py
#12 (comment):

  • tree : skbio.TreeNode
  •    Tree object where the leafs correspond to the features.
    
  • names : list, optional
  •    List of labels to rename the tip names.  It is assumed that the
    
  •    names are listed in level ordering, and the length of the list
    
  •    is at least as long as the number of internal nodes.
    
  • Returns

  • skbio.TreeNode
  •   Tree with renamed internal nodes.
    
  • ValueError:
  •    Raised if `tree` and `name` have incompatible sizes.
    
  • """
  • _tree = tree.copy()

I don't think I follow with the second part of your comment. With the
inplace parameter my comment is resolved.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/biocore/gneiss/pull/12/files/477d1967bb7ee99eb0d4d40d5225e0a62951388a#r72166846,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD_a3ZVo6lBTJ2WocPg7ZNETNTnwLhHwks5qZU0pgaJpZM4JPRmM
.

non_tips = [n for n in _tree.levelorder() if not n.is_tip()]
if not names is None and len(non_tips) != len(names):
if names is not None and len(non_tips) != len(names):
raise ValueError("`_tree` and `names` have incompatible sizes, "
"`_tree` has %d tips, `names` has %d elements." %
(len(non_tips), len(names)))
Expand All @@ -164,7 +164,7 @@ def rename_internal_nodes(tree, names=None):
label = 'y%i' % i
else:
label = names[i]
if not n.name is None and label == n.name:
if n.name is not None and label == n.name:
warnings.warn("Warning. Internal node (%s) has been replaced "
"with (%s)" % (n.name, label))

Expand Down