talgalili / dendextend

Extending R's Dendrogram Functionality
152 stars 28 forks source link

Changed prune_leaf() to prune non-binary trees #76

Closed hypercompetent closed 5 years ago

hypercompetent commented 6 years ago

Hi Tal and contributors,

Thanks for this really excellent package!

I've run into a need to prune non-binary trees recently, and modified the code in prune_leaf() to trim leaves from splits with more than 2 branches. This simply involves removing the list elements from the dendrogram that match the leaf_name to be removed. If this looks useful to you, please merge it in.

Cheers, -Lucas

talgalili commented 6 years ago

Hi @hypercompetent Thank you very much for your commit/PR. Would you be able to: 1) add a test that demonstrates how to use this feature? (in the testthat folder) this would help insure that the code would have the intended functionality also in the future. 2) Update the roxygen2 documentation so to explain this new behavior?

Thanks!

codecov-io commented 5 years ago

Codecov Report

Merging #76 into master will increase coverage by 1.35%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage    35.8%   37.15%   +1.35%     
==========================================
  Files          51       51              
  Lines        3606     3620      +14     
==========================================
+ Hits         1291     1345      +54     
+ Misses       2315     2275      -40
Impacted Files Coverage Δ
R/prune.R 78.21% <100%> (+4.65%) :arrow_up:
R/unbranch.R 81.91% <0%> (+41.48%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 46ca013...658bc43. Read the comment docs.

hypercompetent commented 5 years ago

Sorry for the months-long delay! I've updated the prune.dendrogram() function after uncovering some issues while writing tests.

This should now work for many non-binary dendrograms, and there are now several tests that pass using devtools::test(filter = "trim").

I found that I had to add a call to assign_dendextend_options() to get the tests to work. Otherwise, I got this error related to labels():

 Error in fo(object, ...) : could not find function "fo" 
5. labels.dendrogram(dend)
4. labels(dend) at prune.R#44
3. prune_leaf(dend, leaves[i]) at prune.R#183
2. prune.dendrogram(dend_a, targets_1) at prune.R#166
1. prune(dend_a, targets_1) 
talgalili commented 5 years ago

Hi @hypercompetent Thank you for the work, I've now merged it. (in the future, I should also have asked to have you update the NEWS and the DESCRIPTION, but given that I failed to ask it, I'll do it myself :) )