search

talgalili / dendextend

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

added function that extract au/bp info from pvclust obj (refer to Issues #83) #91

Closed SplitInf closed 5 years ago

SplitInf commented 5 years ago

added function that extract au/bp info from pvclust obj (refer to Issues #83)

codecov-io commented 5 years ago

Codecov Report

Merging #91 into master will increase coverage by 0.08%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   37.37%   37.46%   +0.08%     
==========================================
  Files          51       51              
  Lines        3609     3614       +5     
==========================================
+ Hits         1349     1354       +5     
  Misses       2260     2260
Impacted Files Coverage Δ
R/pvclust.R 3.2% <100%> (+3.2%) :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 f28e912...805af67. Read the comment docs.

SplitInf commented 5 years ago

Thanks for the progress. Several more changes to make:

  1. Please make pvclust.edges be pvclust_edges (the package uses the underscore notation instead of the dot notation).
  2. Please fix the test to run without failing. Based on travis-ci checks the test you've made fails: https://travis-ci.org/talgalili/dendextend/builds/505464846?utm_source=github_status&utm_medium=notification
  3. Please compile the roxygen2 (so there will be an .Rd file for pvclust_edges.
  4. Please add yourself as a contributor to DESCRIPTION, update the version number, the date, and also add a section to NEWS that mentions the addition of the new function.

Thanks!

So test_that works fine on my end, so I suspect travis-ci is giving you issue due to the stochastic nature of pvclust and decimal places. I have omitted the check for au as a temporary workaround. Let me know if you have other suggestions!

talgalili commented 5 years ago

Hi @SplitInf , I can still see the test failing, can you please review and see if this can be fixed? (it might be that you need to add library(pvclust), but I'm not sure. Please check/fix): https://travis-ci.org/talgalili/dendextend/builds/506100177

SplitInf commented 5 years ago

Yes that was it. Glad it works now.

talgalili commented 5 years ago

I've merged your PR and intend to send the latest version to CRAN. Thanks for your contribution.

SplitInf commented 5 years ago

My pleasure. Thanks for making this package available to the community.