Matrix completion for robust CLR in decostand #619
Conversation
|
One thing that jumped out at me is that we are now depending on {testthat} with this PR. I personally don't mind, but @jarioksa might? We've typically avoid running too many tests in the package, and I don't see testthat used anywhere in the tests. |
|
Ah, true. Accidental. I now removed that testthat dependency. DECOID validations still to do but open for other feedback meanwhile. |
|
|
||
| # initialize | ||
| dist = array(0,c(1,(niter+1))) | ||
| dist[1] = norm((M_E - (X%*%S%*%t(Y)))*E,'f')/sqrt(nnZ.E) |
There was a problem hiding this comment.
Formatting again: use spaces around %*% for readability (still quite OK here, but becomes awkward when you have list elements with $-signs.
|
Thanks! We will go through these and update. I would still compare this with results from an independent Python implementation. That currently seems to have broader installation problems. There seem to be some differences at the moment and we are troubleshooting. This will be good to complete before merging. |
|
I'm heading for a CRAN release. It will still take some time: tests are ahead. In good case could be before May Day. Would this suit your plans with CLR? |
|
The main bottleneck currently is that we cannot compare with external Python implementation to make sure the results are comparable. We were waiting for that (external issue) to become fixed. It would be good to get this fixed, I can try to see this week if this could be doable nevertheless. We will report back this week how it looks. Otherwise, it can wait until the next CRAN release. |
|
This PR will be updated & reconsidered after #667 is ready. If we add the matrix completion step in vegan this would be require a new dependency, the "filling" package, which has additional dependencies (CVXR, Rcpp, Rdpack, ROptSpace, RSpectra, nabor, stats, utils). Then the question is whether such new dependency would be acceptable? Entirely reimplementing the filling operation in vegan might not be feasible but we can have a look if it is not possible add new dependencies. |
|
Thanks for the feedback. We are making some final checks and I'm hoping to update & finalize this shortly now. |
|
I am closig this PR. I have taken into account the feedback on this PR and merged the relevant material in PR #667 in the hope that we can finish everything on one go soon. |
This is intended to solve #570
I am opening the PR in order to request feedback before final checks & possible polishing.
Do the maintainers see any shortcomings / issues to be addressed on this?
The matrix completion step is straighforward. The code has been modified from the ROptSpace package. The code was moved to vegan in order to avoid new dependencies, although using the actual original package directly would simplify this and I will happy to change into that if this idea receives support. The ROptSpace package has been cited and its license (MIT) is compatible with vegan's license (GPL) and the code from ROptSpace can be included in vegan.
Unit tests and examples have been updated, too.
Comparisons with an independent Python implementation (deicode) are ongoing, I will check those as well to make sure that this works as expected.
Ah, should the PRs be done againsta vegandevs::master, or another branch?