[Phylobase-devl] improving getNode handling of NA_character_ and missing node IDs

[François Michonneau]

Hi Jim

  Thanks for this.

  Personally I don't have much preference between the mailing or the
tracker. If we had a much more intense level of discussion about bugs
and patches then the tracker would probably better.

  Your patch looks good to me.

  There are indeed, here and there, parts of the code that are
superfluous. I have been trying to clean it up and suppress
redundancies. I hope to have more time to finish and commit some of
these changes soon.

  Cheers,
  -- François

  


On Wed, 2009-06-17 at 15:54 -0700, Jim Regetz wrote:
> Hi all,
> 
> This is a followup to my recent r-forge bug report [#458]. See there for 
> details. Sorry to split between these two modes of communication; my 
> impression is that this list is the way to go for proposing/discussing 
> changes, yet I'm definitely inclined to use the bug tracker for, well, 
> tracking bugs. I'm certainly happy to do things differently in the 
> future if its preferable.
> 
> The attached treewalk.R patch represents my proposed changes to getNode. 
> In this revision, the result vector returns NA values with <NA> names 
> both for nonexistent integer node IDs (i.e., those not occurring in the 
> tree) and for NA characters, and the missingness check works in both cases.
> 
> I've also eliminated what seemed to be superfluous code that struck me 
> as a holdover from when the function relied on a version of labels() 
> that only matched tips. The updated version checks out well in my 
> testing. However, it would be great if someone (Ben, as original 
> author?) could do a sanity check.
> 
> Side question: are folks generally fine with communicating proposed 
> changes in the form of attached patch (unified diff), as I've done here?
> 
> Thanks!
> Jim
> 
> plain text document attachment (treewalk.R.patch)
> Index: treewalk.R
> ===================================================================
> --- treewalk.R	(revision 438)
> +++ treewalk.R	(working copy)
> @@ -12,31 +12,18 @@
>      if (is.numeric(node) && all(floor(node)==node,na.rm=TRUE)) {
>          node <- as.integer(node)
>      }
> -  nolabs <- rep(!hasNodeLabels(phy),length(node))
> +    nt <- nTips(phy)
>      if (is.character(node)) {
> -        ## old getNodeByLabel()
> -        nt <- nTips(phy)
> -        tipmatch <- match(node,labels(phy,"allnode"))
> -        vals <- ifelse(!is.na(tipmatch),
> -                       tipmatch,
> -                       ifelse(nolabs,NA,nt+match(node,nodeLabels(phy))))
> -        names(vals) <- node
> -        rval <- vals
> +        rval <- match(node, labels(phy, "allnode"))
> +        # return NA for any NA_character_ inputs
> +        rval[is.na(node)] <- NA
> +        names(rval) <- node
>      } else if (is.integer(node)) {
> -        ## old getLabelByNode
> -        nt <- nTips(phy)
> -        vals <- ifelse(node<=nt,  ## tips
> -                       labels(phy,"allnode")[node],
> -                       ifelse(node<=nt+nNodes(phy),
> -                              ifelse(nolabs,NA,
> -                                     nodeLabels(phy)[pmax(0,node-nt)]),
> -                              NA))
> -        ## pmax above to avoid error from negative indices
> -        names(node) <- vals
> -        rval <- node
> +        rval <- match(node, seq_len(nt + nNodes(phy)))
> +        names(rval) <- labels(phy,"allnode")[rval]
>      } else stop("node must be integer or character")
>    if (any(is.na(rval))) {
> -    missnodes <- names(rval)[is.na(rval)]
> +    missnodes <- node[is.na(rval)]
>      msg <- paste("some nodes missing from tree: ",paste(missnodes,collapse=","))
>      switch(missing,
>             fail=stop(msg),
> _______________________________________________
> Phylobase-devl mailing list
> Phylobase-devl at lists.r-forge.r-project.org
> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/phylobase-devl