danlwarren / RWTY

R We There Yet?
30 stars 17 forks source link

Problem with running path.dist function #43

Closed NickCrouch closed 9 years ago

NickCrouch commented 9 years ago

Hi,

When I try to run the function topological.autocorr I run into problems with the called function path.dist. In path.dist there are the lines: dt1 = phangorn:::coph(tree1) dt2 = phangorn:::coph(tree2) however the function coph does not appear to be in phangorn. I am using phangorn_1.99-13, rwty_0.1.1, R version 3.1.1 and ape_3.3. I feel like I am missing something obvious!

Any help would be greatly appreciated

Nick

danlwarren commented 9 years ago

It's in phangorn, but it's not exported. This is causing us issues with R CMD check as well, which we'll need to address to be CRAN-ready. For now you can copy and run this code if your system is having trouble finding it:

coph <- function(x){ if (is.null(attr(x, "order")) || attr(x, "order") == "cladewise") x <- reorder(x, "postorder") nTips = as.integer(length(x$tip.label))
parents = as.integer(x$edge[,1]) kids = as.integer(x$edge[,2]) lp= as.integer(length(parents)) nNode = as.integer(x$Nnode) m = as.integer(max(x$edge)) el = double(m) el[kids] = x$edge.length dm <- .C("C_cophenetic", kids, parents, as.double(el), lp, m, nTips, nNode, double(nTips*(nTips-1L)/2L))[[8]] attr(dm, "Size") <- nTips attr(dm, "Labels") <- x$tip.label attr(dm, "Diag") <- FALSE attr(dm, "Upper") <- FALSE class(dm) <- "dist" dm }

NickCrouch commented 9 years ago

Ah! As soon as you pointed it out I saw the extra":". Sorry about that. This is now throwing a new error message, but I will have a play around

danlwarren commented 9 years ago

What message?

NickCrouch commented 9 years ago

I copied the all of the functions from rwty into a new R session, modifying path.dist to read dt1 = coph(tree1) dt2 = coph(tree2) using the function code that you provided that is in phangorn. The error message is now:

 topological.autocorr(fungus)
Error in .C("C_cophenetic", kids, parents, as.double(el), lp, m, nTips,  : 
  C symbol name "C_cophenetic" not in load table
danlwarren commented 9 years ago

Have you loaded phangorn in this new session?

NickCrouch commented 9 years ago

Yes, all package versions as listed previously.

NickCrouch commented 9 years ago

Unfortunately I cannot read C too easily

danlwarren commented 9 years ago

I can't replicate the issue here yet. Given that the code that's pitching the error is actually from phangorn (not rwty), I'm guessing there's some issue with R/rwty seeing your phangorn install. Can you run a couple of other phangorn functions and see if they run normally?

NickCrouch commented 9 years ago

I ran densiTree and dist.ml which seem to run normally. I am looking up the error message in general to see if it is a problem with R/rwty recognizing the phangorn install

NickCrouch commented 9 years ago

I found a posting simply recommending reinstalling the packages, which I did for each in turn, and now the package works with no issues. I successfully ran the analysis on the fungus data set and another sample data set. Very strange. Thank you very much for your comments

danlwarren commented 9 years ago

Oh cool, that's very good to know. Thanks for bringing it up! I'll close this now, but feel free to let us know any other issues you run into. It's very early days with this package, so I'm sure there will be a few. Cheers!

roblanf commented 9 years ago

Hi all,

Glad this is now working, but I'm guessing we should in any case move away from calling non-exported functions. I was a little apprehensive about using it in the first place, since it seems like something one probably shouldn't do. A few options:

  1. Write to Klaus and ask him to export coph
  2. Use another cophenetic distance thing (there is one in ape we could try)
  3. Get Dave S. to code us a solution in pawpty (this is probably best - having full control over the slow bits of the code is always useful)
danlwarren commented 9 years ago

Yep, I totally agree. Solution 2 is easiest in the short term, solution 3 is best long-term.

roblanf commented 9 years ago

Done. We now use the ape cophenetic.phylo function.