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

added tree_subset function #6

Closed
wants to merge 2 commits into from

Conversation

tbradley1013
Copy link

@tbradley1013 tbradley1013 commented May 17, 2018

This PR relates to the discussion in #5. It adds a function tree_subset that takes a tree of class phylo and a selected node of said tree and returns a subset of the tree (still with class phylo) with the relatives of the specified node back to a specified level.

Tests have also been added to ensure that the expected tips and nodes are added and that the branch lengths are preserved during subsetting.

Copy link
Member

@GuangchuangYu GuangchuangYu left a comment

Choose a reason for hiding this comment

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

thanks for the PR. I have commented some of your source codes. Please create a new PR to the treeio package.

"for the development version.",
call. = FALSE)
}

Copy link
Member

Choose a reason for hiding this comment

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

this function should goes to treeio pkg, as it depends on treeio and the output is also a tree object instead of tbl_tree.

Copy link
Member

Choose a reason for hiding this comment

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

another reason is that treeio depends on tidytree and tidytree can't depends on treeio, otherwise there will be a circle dependency and all of these pkgs will fail to compile and install.

dplyr::mutate(isTip = (!node %in% parent)) %>%
dplyr::filter(isTip) %>%
dplyr::pull(label)

Copy link
Member

Choose a reason for hiding this comment

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

do you think group_labels is necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean that the entire code chunk seems unnecessary or it is improperly named? I think I agree if the latter, and will rename it to subset_labels but I do think its functionality is necessary as it is the chunk that is isolating the related nodes

# This drops all of the tips that are not included in group_nodes
subtree <- treeio:::gfocus(tree, group_labels, "focus") %>%
treeio:::drop.tip(., .$tip.label[-group_nodes], rooted = TRUE) %>%
treeio:::groupOTU.phylo(.node = node)
Copy link
Member

Choose a reason for hiding this comment

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

the drop.tip function supports treedata object as well as phylo and it would be better to support treedata object to maintain associated data with nodes in the subtree.

@tbradley1013
Copy link
Author

tbradley1013 commented May 18, 2018

@GuangchuangYu I have started working on the PR for this in the treeio package and switched the implementation to have a method for both phylo and treedata objects to follow the style of the rest of your package. I am running into a slight issue with drop.tip for treedata.

It looks like the tree@data is subset by drop.tip but tree@extraInfo is not. I can try to come up with a work around in tree_subset if that is what desired, but I am not sure if that is something that should just be implemented in drop.tip. The issue I am seeing is that when as_data_frame is called on the subset, the extraInfo does not match correctly and all the additional rows without matching nodes are just appended to the end.

Other than that, the PR is almost ready to submit, but I will wait for your feedback before I move forward with it.

@GuangchuangYu
Copy link
Member

@tbradley1013 thanks for pointing this out. With the commit, YuLab-SMU/treeio@1c8627e, drop.tip now works with tree@extraInfo.

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.

2 participants