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

branch.length parameter to work with phylo4d #102

Merged
merged 2 commits into from
Dec 7, 2016

Conversation

brj1
Copy link
Contributor

@brj1 brj1 commented Dec 1, 2016

I modified the fortify.phylo4d function to enable the branch.length parameter.

I also wrote a get.tree function for the data.frame type.

function(object, ...) {
edge <- object[, c("parent", "node")]
edge <- edge[edge[,1] != 0 & edge[,1] != edge[,2], ]
edge.length <- object[, "branch.length"]
Copy link
Member

@GuangchuangYu GuangchuangYu Dec 2, 2016

Choose a reason for hiding this comment

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

i <- which(edge[,1] != 0 & edge[,1] != edge[,2])
edge <- edge[i, ]
edge.length <- object[i, "branch.length"]

otherwise edge.length will longer than nrow(edge)

@@ -394,11 +394,15 @@ fortify.phylo4 <- function(model, data, layout="rectangular", yscale="none",
##' @method fortify phylo4d
##' @export
fortify.phylo4d <- function(model, data, layout="rectangular", yscale="none",
ladderize=TRUE, right=FALSE, mrsd=NULL, ...) {
ladderize=TRUE, right=FALSE, branch.length="branch.length",
Copy link
Member

Choose a reason for hiding this comment

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

If you look careful for other methods, you will find they all missing the branch.length parameter.

This is not needed as it will be passed by ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The branch.length parameter may be passed by ..., but it is ignored later on in the chain. The change I implemented allows ggtree to scale branch lengths of phylo4d objects.

@GuangchuangYu
Copy link
Member

GuangchuangYu commented Dec 2, 2016

please correct it and I will accept your get.tree method for data.frame.

you can also update DESCRIPTION by adding:

person("first name", "last name", email = "email", rol = "ctb",
	          comment = "get.tree method for data.frame")

I want to acknowledge all the contributors.

@GuangchuangYu
Copy link
Member

branch.length actually works.

taking the example from #47:

ggtree(g2, branch.length='none')

will drop the branch length and only show cladogram.

Pls show me a not working example.

@brj1
Copy link
Contributor Author

brj1 commented Dec 5, 2016

Try:

library(ggtree); library(ape); library(phylobase); tree <- rtree(5); ptree <- phylo4d(tree, all.data=data.frame(eq=rep(1, 9))); ggtree(ptree, branch.length="eq")

@GuangchuangYu
Copy link
Member

GuangchuangYu commented Dec 6, 2016

good catch. But your solution is not a good idea by converting tree object to data.frame and convert data.frame back to phylo.

this issue had been fixed and now you can set branch.length to feature available in phylo4d@data.

@GuangchuangYu
Copy link
Member

GuangchuangYu commented Dec 6, 2016

If you are serious to make PR, you need to be careful of not introducing new bugs by testing your code (e.g. get.tree contains obvious (although fixed) bug, and your PR won't compile (invalid DESCRIPTION file, you even don't try to build the pkg) etc).

@brj1
Copy link
Contributor Author

brj1 commented Dec 6, 2016

Thanks for adding branch.length support for phylo4d. I rewrote my repository to only containn the get.tree function for data.frame. It now builds and I think is ready to be pulled. Sorry about the sloppy coding.

@GuangchuangYu GuangchuangYu merged commit f59b997 into YuLab-SMU:master Dec 7, 2016
@GuangchuangYu
Copy link
Member

I find you remove edge <- object[, c("parent", "node")] in your PR and make your code broken.

It was fixed in 1a0e6ac.

FYI, all the tree IO utilities will be removed when treeio was accepted to deposit on Bioconductor.

Your get.tree method for data.frame was incorporated in treeio.

It would be better to have unit test, like https://github.com/GuangchuangYu/treeio/blob/master/tests/testthat/test-as-phylo.R.

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