Striped Unifrac C++ implementation#829
Conversation
The C++ code works for rooted trees, but not unrooted ones. Possibly a bug in my implementation, more testing needed.
Some datasets still produce divergent values. Likely a bug in my implementation.
Bringing the assay into C++ was using rowTree tip labels for the observation ids, causing nonsense results when they were in different order from the actual rownames. Also added a check for cladewise tree ordering.
However, speed is slower than current implementation
Unweighted is now significantly faster and less memory intensive than the existing implementation
Merge remote-tracking branch 'upstream/devel' into fastunifrac # Conflicts: # DESCRIPTION # NAMESPACE # R/RcppExports.R # R/addAlpha.R # src/.gitignore # src/RcppExports.cpp # src/assay.cpp # src/assay.h # src/propmap.cpp # src/propmap.h # src/tree.h
|
Excellent. Do you have the code available to demonstrate the ecodive comparisons (or could we include it in the unit tests)? This might be helpful for later. |
|
The current tests in test-5Unifrac.R already compare the results against ecodive, so that part should already be covered. The datasets used there aren't big enough for there to be any large differences in runtime, but benchmarking should also be possible by taking those comparisons and timing them. |
|
Cool! @TuomasBorman If this can be checked and merged, I'll try to include it in the OMA benchmark. |
TuomasBorman
left a comment
TuomasBorman
left a comment
There was a problem hiding this comment.
Thanks, looks good! I will check this this week in more detail
| #' @importFrom ecodive unweighted_unifrac | ||
| .get_unifrac <- function( | ||
| x, tree, weighted = FALSE, node.label = nodeLab, nodeLab = NULL, ...){ | ||
| x, tree, new = FALSE, weighted = FALSE, node.label = nodeLab, nodeLab = NULL, ...){ |
There was a problem hiding this comment.
Where this "new" goes? What is the purpose?
There was a problem hiding this comment.
Same here. Try to avoid making changes that are not strictly needed
4 participants
As discussed in #756, this code adds a C++ implementation of the Striped Unifrac algorithm (https://www.nature.com/articles/s41592-018-0187-8, adapted from the supplementary code in the article), extending the code of the previously added C++ Faith index implementation (#522).
Results should be identical to those provided by ecodive for both weighted and unweighted cases, and at least based on my own testing on a normal laptop somewhat faster and less memory intensive. Further improvements could be likely be made by adding support for multithreading and more optimization. It would also be fairly easy to adapt the code for other forms of unifrac such as generalized unifrac if desired.