@agisga review

[lrnv]

The review from @agisga:

I'm deeply sorry for the delay in the review (the timing turned out to be very far from ideal for me unfortunately). Thank you for the patience.

I think that cort is quality software and a substantial scholarly effort, and I would suggest acceptance subject to minor revisions correcting the issues outlined in this thread.
Some of the issues that I came across were already mentioned by @coatless above, most importantly, the lack of community guidelines, instructions for potential contributors, for issue reporting or other support. So, I will try not to repeat what has already been said.

Vignettes

In addition to what @coatless has mentioned, the second code block in vignette 4 (https://cran.r-project.org/web/packages/cort/vignettes/vignette04_bootstrap_varying_m.html) has the following redundant lines that should be removed:

        test <- 
        train <- 

Automated tests

I get some warnings when running the automated tests manually with devtools::test (tested with R version 4.0.3 (2020-10-10) on Arch Linux):

test-CortForest.R:5: warning: (unknown)
UNRELIABLE VALUE: Future ('<none>') unexpectedly generated random numbers without specifying argument '[future.]seed'. There is a risk that those random numbers are not statistically sound and the overall results might be invalid. To fix this, specify argument '[future.]seed', e.g. 'seed=TRUE'. This ensures that proper, parallel-safe random numbers are produced via the L'Ecuyer-CMRG method. To disable this check, use [future].seed=NULL, or set option 'future.rng.onMisuse' to "ignore".

test-CortForest.R:9: warning: (unknown)
UNRELIABLE VALUE: Future ('<none>') unexpectedly generated random numbers without specifying argument '[future.]seed'. There is a risk that those random numbers are not statistically sound and the overall results might be invalid. To fix this, specify argument '[future.]seed', e.g. 'seed=TRUE'. This ensures that proper, parallel-safe random numbers are produced via the L'Ecuyer-CMRG method. To disable this check, use [future].seed=NULL, or set option 'future.rng.onMisuse' to "ignore".

While this is very minor issue, I think such warnings should be fixed (or maybe explicitly set to be ignored) unless there are reasons not to.

State of the field

State of the field: Do the authors describe how this software compares to other commonly-used packages?

I'm not very familiar with estimation of non-parametric copulas and so this may not be relevant, but I don't really see a comparison to other R or Python packages. A simple web search shows that other copula estimation packages exist. While the cort paper mentions publications by Li et al. and Nagler et al. as alternative approaches, it is not clear whether these papers correspond to some commonly-used software packages or are mainly theoretical/methodological papers.

Quality of writing

There are some minor grammatical and spelling mistakes in the paper. So, a proofreading would be beneficial. Here are some of the mistakes I noticed:

• Page 1, third paragraph:

  There also exists several tree-structured piecewise constant density estimators,

  "exists" should be "exist"

  The new models that are implemented in this package tries to solve these issues.

  "tries" should be 'try"

• Page 2:

  • "pakcage" should be "package"

  • "was design" should be "was designed"

• Page 2:

  "Examples datasets are included in the package, and the many vignettes gives examples of usecases."

  "Examples" should be "Example", "gives" should be "give", "usecases" should be "use cases".

Originally posted by @agisga in openjournals/joss-reviews#2653 (comment)

[lrnv]

Splitted into smaller issues