search

zpneal / backbone

R backbone package - Extract the backbone from weighted and unweighted networks
https://www.rbackbone.net
40 stars 8 forks source link

Fix Bug with tomatrix #33

Closed delabj closed 3 years ago

delabj commented 3 years ago

I was able to get the package working locally with this fork.

I believe this closes #32, after review of course.

I also updated tests to allow this to run on my machine. Tests as written wouldn't have worked since the class of a single object will be of length 1 and a list of 2 is larger so expect equal would always fail.

zpneal commented 3 years ago

I've had a chance to review the pull request. Your change to model.helpers.R, setting stringsAsFactors = FALSE seems to make tomatrix() work with versions of R before 4.0, so we'll want to implement that. However, I think your edits to the tests for tomatrix will cause problems. In R 4.0, class(test$G) will return "matrix" "array", but in earlier versions of R it would only return "matrix". Since the tomatrix() function is just a minor helper function, it may be better to just eliminate the tests, rather than trying to make them backward and forward compatible. Also, just a style preference - we reserve domagal9:master for the current CRAN release, so prefer pull requests are submitted to domagal9:develop.

zpneal commented 3 years ago

I've made the suggested change to model.helpers.R, and have updated the tests for tomatrix(). These changes are implemented in the develop branch. So, if you install using install_github("domagal9/backbone", ref = "develop"), it should be working now for R before 4.0. I'm closing this request, but please let me know if you get more errors related to this.

delabj commented 3 years ago

Good to know the style preference, I'll keep it in mind if I have any future stuff! Thanks, for the package!