Best practices for R packages not consistently followed

[fabian-s]

I see multiple issues in terms of missing documentation and poor namespace management. Please make sure your package goes through R CMD check / devtools::check cleanly.

> devtools::check("compareMCMCs")
[....]

── R CMD check results ───────────────────────────────── compareMCMCs 0.5.0 ────
Duration: 1m 23.3s

❯ checking dependencies in R code ... WARNING
  '::' or ':::' imports not declared from:
    ‘coda’ ‘rjags’ ‘rstan’
  'loadNamespace' or 'requireNamespace' calls not declared from:
    ‘coda’ ‘rjags’ ‘rstan’
  'library' or 'require' calls to packages already attached by Depends:
    ‘nimble’ ‘reshape2’
    Please remove these calls from your code.
  Packages in Depends field not imported from:
    ‘R6’ ‘ggplot2’ ‘grid’ ‘nimble’ ‘reshape2’ ‘xtable’
    These packages need to be imported from (in the NAMESPACE file)
    for when this namespace is loaded but not attached.
  Unexported object imported by a ':::' call: ‘nimble:::removeIndexing’
    See the note in ?`:::` about the use of this operator.

❯ checking for missing documentation entries ... WARNING
  Undocumented code objects:
    ‘MCMCmetric_efficiency’ ‘getMetrics’ ‘getPageComponents’
    ‘registerMetrics’ ‘registerPageComponents’ ‘unregisterMetric’
    ‘unregisterPageComponents’
  All user-level objects in a package should have documentation entries.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.

❯ checking Rd \usage sections ... WARNING
  Undocumented arguments in documentation object 'combineMetrics'
    ‘include_times’

  Undocumented arguments in documentation object 'compareMCMCs'
    ‘metricOptions’ ‘conversions’ ‘needRmodel’ ‘verbose’

  Undocumented arguments in documentation object 'metrics'
    ‘...’

  Undocumented arguments in documentation object 'modifyMetrics'
    ‘options’

  Functions with \usage entries need to have the appropriate \alias
  entries, and all their arguments documented.
  The \usage entries must correspond to syntactically valid R code.
  See chapter ‘Writing R documentation files’ in the ‘Writing R
  Extensions’ manual.

❯ checking package dependencies ... NOTE
  Depends: includes the non-default packages:
    'R6', 'nimble', 'ggplot2', 'grid', 'reshape2', 'xtable'
  Adding so many packages to the search path is excessive and importing
  selectively is preferable.

❯ checking DESCRIPTION meta-information ... NOTE
  License components which are templates and need '+ file LICENSE':
    BSD_3_clause
  Package listed in more than one of Depends, Imports, Suggests, Enhances:
    ‘nimble’
  A package should be listed in only one of these fields.

❯ checking top-level files ... NOTE
  Non-standard file/directory found at top level:
    ‘schools.stan’

❯ checking R code for possible problems ... NOTE
  MCMCdef_dummy_impl: no visible global function definition for ‘rnorm’
  MCMCdef_jags_impl: no visible global function definition for ‘update’
  MCMCdef_jags_impl: no visible binding for '<<-' assignment to
    ‘monitorNodesBUGS’
  MCMCdef_jags_impl: no visible binding for global variable
    ‘monitorNodesBUGS’
  MCMCmetric_CI95low: no visible binding for global variable ‘quantile’
  MCMCmetric_CI95upp: no visible binding for global variable ‘quantile’
  MCMCmetric_median: no visible binding for global variable ‘median’
  MCMCmetric_sd: no visible binding for global variable ‘sd’
  allParamEfficiencyComparisonComponent: no visible global function
    definition for ‘aggregate’
  compareMCMCs: no visible global function definition for ‘nimbleModel’
  make_MCMC_comparison_pages: no visible global function definition for
    ‘jpeg’
  make_MCMC_comparison_pages: no visible global function definition for
    ‘dev.off’
  runNIMBLE: no visible global function definition for ‘buildMCMC’
  runNIMBLE: no visible global function definition for ‘compileNimble’
  Undefined global functions or variables:
    aggregate buildMCMC compileNimble dev.off jpeg median
    monitorNodesBUGS nimbleModel quantile rnorm sd update
  Consider adding
    importFrom("grDevices", "dev.off", "jpeg")
    importFrom("stats", "aggregate", "median", "quantile", "rnorm", "sd",
               "update")
  to your NAMESPACE file.

[fabian-s]

additionally from goodpractice::gp():

✖ use '<-' for assignment instead of '='. '<-' is the standard, and R users
    and developers are used it and it is easier to read your code for them if you use
    '<-'.

    R/make_MCMC_comparison_pages.R:244:7
    R/make_MCMC_comparison_pages.R:315:6
    R/make_MCMC_comparison_pages.R:324:6
    R/make_MCMC_comparison_pages.R:426:6
    R/make_MCMC_comparison_pages.R:434:6

  ✖ avoid long code lines, it is bad for readability. Also, many people prefer
    editor windows that are about 80 characters wide. Try make your lines shorter than
    80 characters

    R/compareMCMCs.R:9:1
    R/compareMCMCs.R:28:1
    R/compareMCMCs.R:31:1
    R/compareMCMCs.R:33:1
    R/compareMCMCs.R:47:1
    ... and 197 more lines

  ✖ avoid calling setwd(), it changes the global environment. If you need it,
    consider using on.exit() to restore the working directory.

    R/make_MCMC_comparison_pages.R:101:3
    R/make_MCMC_comparison_pages.R:102:11

  ✖ avoid the library() and require() functions, they change the global search
    path. If you need to use other packages, import them. If you need to load them
    explicitly, then consider loadNamespace() instead, or as a last resort, declare them
    as 'Depends' dependencies.

    R/metrics-addMetrics.R:51:7
    R/runNIMBLE.R:8:3

[perrydv]

Thanks @fabian-s . I have fixed up all of these best practices recommendations. The only exception is the warning on setwd, which recommends pairing it with on.exit if really needed. That is the situation here, and we did have it paired with on.exit to reset the directory already. I think that one needs to stay, so one warning from goodpractice::gp() remains for that reason.

[fabian-s]

using branch joss at https://github.com/nimble-dev/compareMCMCs/commit/ace0287fa1decc3489fcc85c74768d8be8be0e67, I got:

1. an error trying to rebuild the vignettes, that was easy to fix like this::

--- a/compareMCMCs/vignettes/compareMCMCs.Rmd
+++ b/compareMCMCs/vignettes/compareMCMCs.Rmd
@@ -22,9 +22,9 @@ knitr::opts_chunk$set(
   collapse = TRUE,
   comment = "#>"
 )
-this_system_has_rjags <- requireNamespace(rjags)
+this_system_has_rjags <- requireNamespace("rjags")
 if(!this_system_has_rjags) message("Portions of this vignette use package rjags.  That is not installed, so those portions will be skipped.")
-this_system_has_rstan <- requireNamespace(rstan)
+this_system_has_rstan <- requireNamespace("rstan")
 if(!this_system_has_rstan) message("Portions of this vignette use package rstan.  That is not installed, so those portions will be skipped.")

2. after this fix, R CMD check still complains about:

   
   ── R CMD check results 
   ──────────────────────────── compareMCMCs 0.5.0 ────
   Duration: 1m 18.7s

checking dependencies in R code ... WARNING '::' or ':::' imports not declared from: ‘rjags’ ‘rstan’ 'loadNamespace' or 'requireNamespace' calls not declared from: ‘rjags’ ‘rstan’ Package in Depends field not imported from: ‘nimble’ These packages need to be imported from (in the NAMESPACE file) for when this namespace is loaded but not attached.

checking for code/documentation mismatches ... WARNING Codoc mismatches from documentation object 'compareMCMCs': compareMCMCs Code: function(modelInfo = list(), MCMCcontrol = list(niter = 10000, thin = 1, burnin = 2000), MCMCs = "nimble", monitors = character(), nimbleMCMCdefs = list(), externalMCMCinfo = list(), metrics = c("mean", "median", "sd", "CI95_low", "CI95_upp", "efficiency_coda"), metricOptions = list(), conversions = list(), seed = NULL, needRmodel, verbose = TRUE, sessionInfo = TRUE) Docs: function(modelInfo = list(), MCMCcontrol = list(niter = 10000, thin = 1, burnin = 2000), MCMCs = "nimble", monitors = character(), nimbleMCMCdefs = list(), externalMCMCinfo = list(), metrics = c("mean", "median", "sd", "CI95_low", "CI95_upp", "efficiency_coda"), metricOptions = list(), conversions = list(), seed = NULL, needRmodel, verbose = TRUE, sessionInfo = TRUE, gc = TRUE) Argument names in docs not in code: gc

checking R code for possible problems ... NOTE MCMCdef_jags_impl: no visible global function definition for ‘update’ Undefined global functions or variables: update Consider adding importFrom("stats", "update") to your NAMESPACE file.

checking for unstated dependencies in vignettes ... NOTE 'library' or 'require' calls not declared from: ‘rjags’ ‘rstan’

please repair remaining issues with dependencies, imports and method definitions and make sure your vignettes' code is executable..    

[perrydv]

Thanks @fabian-s. It was in an intermediate state last time you prodded it. For me it now passes R CMD check cleanly and the vignette builds cleanly.

[fabian-s]

Awesome, looking great now!