rikenbit / iTensor

R package for ICA-based Matrix/Tensor Decomposition
Other
1 stars 1 forks source link

[JOSS REVIEW] Test coverage is insufficient #4

Closed devarops closed 1 year ago

devarops commented 1 year ago

@kokitsuyuzaki:

The current test coverage for the code is approximately 35.8%, which falls short of providing sufficient verification of the software's functionality and justifiably recommending its acceptance.

My suggestion is to strive for a minimum coverage of 50% in each file or, ideally, attain a target of 90% coverage throughout the entire repository. Through this approach, we can considerably enhance the reliability and robustness of the software.

kokitsuyuzaki commented 1 year ago

Thank you for recommending that. For toyModel (0%) and CorrIndex (0%), I didn't write tests so 0% is very reasonable. I added test-toyModel.R and test-CorrIndex.R, so please check them. https://github.com/rikenbit/iTensor/tree/main/tests/testthat

Using covr package, I could confirm that the current coverage is like below:

> library("covr")
> (cov <- package_coverage("iTensor"))
iTensor Coverage: 58.06%
R/ICA2.R: 27.65%
R/ICA.R: 65.09%
R/MICA.R: 99.27%
R/GroupICA.R: 99.53%
R/CorrIndex.R: 100.00%
R/MultilinearICA.R: 100.00%
R/toyModel.R: 100.00%

I spent a day trying to increase the coverages of ICA and ICA2, but could not do it. Perhaps this is because these functions make heavy use of hidden functions. In fact, everything in zero_coverage was about hidden functions.

> zero_coverage(cov)
         filename                             functions line value
788  R/GroupICA.R                   .generate_subgroups  336     0
342       R/ICA.R                            .whitening  118     0
391       R/ICA.R                              .FastICA  136     0
...
547      R/MICA.R          .der_H_beta_i_der_beta_i_k_l  194     0

I think these low coverages caused by hidden functions are on the covr side rather than the comprehensiveness of my tests. In fact, all these functions are already called and evaluated by other functions and should not be output by zero_coverage. I also modified the tests to explicitly call and evaluate such hidden functions but did not improve these coverage values.

As there is nothing more I can do, may I ask you to close the Issue in this state?

devarops commented 1 year ago

Hello @kokitsuyuzaki,

I noticed that there is a failing test in line 41 of test-MICA.R. Please make the necessary modifications to ensure that all tests pass.

Once all tests pass, please let me know so I can continue with my review.

image

kokitsuyuzaki commented 1 year ago

Thanks, I modified the test-MICA.R. https://github.com/rikenbit/iTensor/commit/72001ed7defe6341ce4411ec348f5a1432f2d791

devarops commented 1 year ago

Thank you, @kokitsuyuzaki!