fix plotting #63
fix plotting #63
Conversation
and add a nodedepths function
a function to count the number of non-leaf (internal) nodes
Pull Request Test Coverage Report for Build 1339497475
💛 - Coveralls |
| Returns the number of internal nodes of a single tree, or a Dict of numbers of nodes | ||
| for multiple trees. | ||
| """ | ||
| ninternal(tree::AbstractTree) = nnodes(tree) - nleaves(tree) |
There was a problem hiding this comment.
Is the root node internal?
There was a problem hiding this comment.
Yes!
What I'm trying to address here is that people coming from R's ape library will be confused, because it distinguishes between "tips" (which you call "leaves") and "nodes" which are all the nodes that aren't tips! Phylo doesn't seem to have a concept for that, but I use it a lot - so that's why I suggest the name "internal" for these.
There was a problem hiding this comment.
So ape:::Nnode(tree) + ape:::Ntip(tree) in R equals Phylo.nnodes(tree)!
There was a problem hiding this comment.
Cool, okay. As I've said often, I'm not a phylogeneticist and I just wasn't sure whether the root was internal, but it obviously is...
src/metrics.jl
Outdated
| function nodedepths(tree::Phylo.AbstractTree) | ||
| function finddepths!(clade::String, parentdepth::Float64 = 0.0) | ||
| mydepth = parentdepth | ||
| push!(names, clade) | ||
| if hasinbound(tree, clade) | ||
| mydepth += getlength(tree, getinbound(tree, clade)) | ||
| end | ||
| depth[clade] = mydepth | ||
| for ch in getchildren(tree, clade) | ||
| finddepths!(ch, mydepth) | ||
| end | ||
| end | ||
|
|
||
| depth = Dict{String, Float64}() | ||
| names = String[] | ||
| sizehint!(depth, nnodes(tree)) | ||
| sizehint!(names, nnodes(tree)) | ||
| root = getnodename(tree, getroot(tree)) | ||
| finddepths!(root) | ||
| depth, names |
There was a problem hiding this comment.
I haven't had time to check, but isn't this just heightstoroot()?
Line 214 in 61af6c7
There was a problem hiding this comment.
Not exactly - nodedepths also includes internal nodes, heightstoroot is only for tips/leaves (although I agree that's not clear from the names). nodedepths is also quite a bit faster
julia> @btime heightstoroot(hummers)
2.311 ms (4247 allocations: 383.22 KiB)
julia> @btime nodedepths(hummers)
633.794 μs (2771 allocations: 111.23 KiB)There was a problem hiding this comment.
Okay, in that case can we move distance(), distances(), heighttoroot() and heightstoroot() into this file (I didn't really notice it appearing a few months ago) and rewrite heightstoroot() to call this? It seems unwise to have two functions with such overlapping functionality. We also ought to decide on whether we are going to call this measurement height or depth... we shouldn't use both.
There was a problem hiding this comment.
I could rename _nodeheights to _nodedepths, nodedepths to nodeheights and heightstoroot to leafheights for consistency though?
There was a problem hiding this comment.
I don't really understand what you're saying here. I was wondering if we could agree on a single name for distances inside the tree, but you seem to be suggesting switching heights and depths in different contexts?
There was a problem hiding this comment.
@mkborregaard - I appreciate you're in a hurry! Why don't you do the renames you suggest, revise nodeheights() function with a leavesonly keyword, and just copy the heightstoroot() function across for now but don't do anything with it. If Claire's okay with it, we'll deprecate that, and if not, we'll rewrite it later to call nodeheights().
There was a problem hiding this comment.
For now I could just keep my function nodeheight to do what it does and we can always deprecate heightstoroot later if it becomes redundant. I can unexport nodeheight for now to not confuse the interface.
There was a problem hiding this comment.
Oh, sorry - just saw your response. I wouldn't go with the symbols option, but I'm neutral about the other option. We could also add in a keyword to say what the height is relative to. I think all of these may be refinements for later though. The simpler answer would be just your leavesonly keyword - I'm happy with that for now. I'm (honestly!) trying not to add too much work here :)
There was a problem hiding this comment.
I'd keep nodeheights() exported and add the keyword and we'll (very likely) deal with heightstoroot() later if Claire is okay with it being deprecated.
There was a problem hiding this comment.
OK, I've changed the names, and added onlyleaves and noleaves keywords - I guess it would be good to standardize on some keywords to use everywhere but we can discuss that. I implement the keywords by filtering after the fact, because we need to calculate all the values anyway.
I also return an AxisArray now for consistency. It makes the function run like 50% slower, which I guess isn't a disaster if it's important to get an AxisArray out.
I have also reimplemented heightstoroot to just be nodeheights(tree, onlyleaves = true), which makes it 15 times faster on the h1n1 tree while giving the exact same result.
julia> @btime heightstoroot(tree1) # old
161.813 ms (39269 allocations: 3.51 MiB)
julia> @btime heightstoroot(tree1) # new
11.970 ms (13699 allocations: 562.05 KiB)
|
Ready to merge |
src/metrics.jl
Outdated
| end | ||
|
|
||
| """ | ||
| height(tree::AbstractTree, node) |
There was a problem hiding this comment.
Bug in the original docs:
| height(tree::AbstractTree, node) | |
| heighttoroot(tree::AbstractTree, node) |
src/metrics.jl
Outdated
| end | ||
|
|
||
| """ | ||
| heights(tree::AbstractTree) |
There was a problem hiding this comment.
Bug in the original docs:
| heights(tree::AbstractTree) | |
| heightstoroot(tree::AbstractTree) |
src/plot.jl
Outdated
| @@ -292,7 +292,7 @@ end | |||
|
|
|||
|
|
|||
| # a function to update a value successively from the root to the tips | |||
| function map_depthfirst(FUN, start, tree, eltype = nothing) | |||
| function map_heightfirst(FUN, start, tree, eltype = nothing) | |||
There was a problem hiding this comment.
Despite preferring heights for trees, I don't think anyone would fine heightfirst more intuitive than depthfirst for a search algorithm:
| function map_heightfirst(FUN, start, tree, eltype = nothing) | |
| function map_depthfirst(FUN, start, tree, eltype = nothing) |
It's not clear to me this function is used incidentally.
This has been languishing for some time.
I think I deleted the wrong branch, so my old PR disappeared, so I'm making a new one.
I'm adding a number of PRs now. I'm doing them sequentially - they build on each other - so they should also be merged and rebased sequentially. Let me know when / if I can merge.