rrrlw / TDAstats

R pipeline for computing persistent homology in topological data analysis. See https://doi.org/10.21105/joss.00860 for more details.
https://rrrlw.github.io/TDAstats
GNU General Public License v3.0
37 stars 8 forks source link

modified calculate_homology to work with dist #10

Closed ShotaOchi closed 5 years ago

ShotaOchi commented 5 years ago

I modified calculate_homology function to work with dist class object. Now, mat will be converted to matrix when mat is dist class object. The point of modification is shown below.

  # convert mat to matirx if class of mat is dist
  if (any(class(mat) == "dist")) {
    mat <- as.matrix(mat)
  }
codecov-io commented 5 years ago

Codecov Report

Merging #10 into master will increase coverage by 0.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.01%     
==========================================
  Files           6        6              
  Lines         563      564       +1     
==========================================
+ Hits          507      508       +1     
  Misses         56       56
Impacted Files Coverage Δ
R/calculate.R 96.66% <100%> (+0.11%) :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 bacf7f2...217c905. Read the comment docs.

rrrlw commented 5 years ago

Thank you for submitting this pull request, it looks like a good fix for the issue brought up in #1. Given that it passes both CIs (Travis and AppVeyor) without issues, I am ready to accept it. A quick question before I do: do you think it would be helpful to try mat <- as.matrix(mat) irrespective of the initial class of mat?

My thought is that if mat is a matrix, no harm done and if mat is of any other class (including not the dist class) but can be coerced into a numeric matrix, then those problems would be fixed too. What do you think, @ShotaOchi?

ShotaOchi commented 5 years ago

I think mat <- as.matrix(mat) irrespective of the initial class of mat is helpful.

We can construct distance matrices using distances function of distances package, and we fail to use calculate_homology function with the distance matrices made by distances function. In case of distances function of distances package, the solution is same as the one of dist class object. mat <- as.matrix(mat) before using calculate_homology function fixes the problem.

mat <- as.matrix(mat) fixes the problems caused by class of mat (dist class and distances class). That's why I believe mat <- as.matrix(mat) irrespective of the initial class of mat will fix problems caused by class of mat.

rrrlw commented 5 years ago

Looks good, just merged; thank you @ShotaOchi!