search

liamrevell / phytools

GNU General Public License v3.0
207 stars 56 forks source link

getDescendants not fetching all descendants? #105

Closed HedvigS closed 1 year ago

HedvigS commented 2 years ago

Hi Liam.

I'm experiencing unexpected behavior from getDescendants, I was wondering if you know what's up. I'm giving it two nodes, and it should find 5 descendants but it only finds 4. I included a demonstration with phangorn::Descendants() which does find all 5.

library(phytools)
library(phangorn)
library(ape)

tree_string <- "(((((agua1252:1,ixil1251:1)ixil1250:1,(mamm1241:1,tekt1235:1)mame1240:1)grea1277:1,(((kaqc1270:1,tzut1248:1)cakc1244:1,(achi1256:1,kich1262:1)quic1275:1,saca1238:1,sipa1247:1)core1251:1,kekc1242:1,(poqo1253:1,poqo1254:1)poco1241:1,uspa1245:1)grea1276:1)quic1274:1,((((chol1282:1,(buen1245:1,mira1253:1,tamu1247:1)taba1266:1)chol1281:1,(chol1283:1,chor1273:1)chor1272:1,epig1241:1)chol1287:1,((chan1320:1,tena1239:1)tzel1254:1,tzot1259:1)tzel1253:1)chol1286:1,((chuj1250:1,tojo1241:1)chuj1249:1,((west2635:1,popt1235:1,qanj1241:1)kanj1263:1,(moto1243:1,tuza1238:1)moch1257:1)kanj1262:1)kanj1261:1)west2865:1,((itza1241:1,mopa1243:1)mopa1242:1,((laca1244:1,naja1242:1)laca1243:1,yuca1254:1)yuca1253:1)yuca1252:1)core1254:1,(chic1271:1,huas1242:1)huas1241:1)maya1287:1;"

tree <- ape::read.tree(text = tree_string)

phytools::getDescendants(tree, node = c(59, 65)) 
phangorn::Descendants(tree, node = c(59, 65),type = "all") %>% unlist()
HedvigS commented 2 years ago

Oh, just noticed. phangorn::Descendants() doesn't report node 66. Huh...

HedvigS commented 2 years ago

I just gotta go type = "all". My bad, all good now for the phangorn funciton.

HedvigS commented 2 years ago

Sorry that the original code snippet above was a bit messy an inaccurate, I've updated it now.

HedvigS commented 2 years ago

I think I get it now!! phytools::getDescendants() only operates on one node at a time! And if I give it more than one, it just checks the first one in the list. However, phangorn::Descendants() can do multiple nodes at a time!

HedvigS commented 2 years ago

Okay, maybe that's super obvious to all other users but it took me a while to get. Haha! Could the documentation maybe be updated? For slow people like me :)?

liamrevell commented 1 year ago

Dear @HedvigS. Just catching up on this now. I can see that you worked it out. The doc page for getDescendants says:

node: an integer specifying a node number in the tree.

I'm glad that you were able to work it out without my feedback.

All the best, Liam

HedvigS commented 1 year ago

Thanks @liamrevell ! I'm sorry for accidentally making your github issues my rubber duck 🐥.

In case other users are like me, could the documentation say

node: an integer specifying a node number in the tree. (Note: the function only takes one node at a time.)