Merge pull request #332 from insightsengineering/fix-tests

Fix tests
insightsengineering · Mar 17, 2022 · 08e6dce · 08e6dce
2 parents 539d258 + 3ea39a9
commit 08e6dce
Show file tree

Hide file tree

Showing 13 changed files with 136 additions and 57 deletions.
diff --git a/.vscode/tasks.json b/.vscode/tasks.json
@@ -7,7 +7,7 @@
             "label": "Build Vignettes",
             "type": "shell",
             "command": "Rscript vignettes/build.R",
-            "problemMatcher": {},
+            "problemMatcher": [],
             "presentation": {
                 "echo": true,
                 "reveal": "always",
@@ -16,6 +16,45 @@
                 "showReuseMessage": true,
                 "clear": true
             }
+        },
+        {
+            "label": "R - testthat (nightly)",
+            "command": "Rscript",
+            "options": {
+                "env": {
+                    "R_TEST_NIGHTLY" : "TRUE"
+                }
+            },
+            "args": [
+                "-e",
+                "devtools::test()"
+            ],
+            "group": "test",
+            "presentation": {
+                "clear": true,
+                "panel": "dedicated"
+            },
+            "problemMatcher": {
+                "owner": "R-testthat",
+                "severity": "error",
+                "fileLocation": [
+                    "relative",
+                    "${workspaceFolder}/tests/testthat"
+                ],
+                "pattern": [
+                    {
+                        "regexp": "^(Failure|Error)\\s\\((.*\\.[Rr]):(\\d+):(\\d+)\\):\\s(.*)",
+                        "file": 2,
+                        "line": 3,
+                        "column": 4,
+                        "message": 5
+                    },
+                    {
+                        "regexp": "^(.*)$",
+                        "message": 1
+                    }
+                ]
+            }
         }
     ]
 }
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: rbmi
 Title: Reference Based Multiple Imputation
-Version: 1.1.1
+Version: 1.1.2
 Authors@R: c(
     person("Craig", "Gower-Page", email = "[email protected]", role = c("aut", "cre")),
     person("Alessandro", "Noci", email = "[email protected]", role = c("aut")),
@@ -11,6 +11,8 @@ Description: Implements reference based multiple imputation allowing for the imp
 Encoding: UTF-8
 LazyData: true
 Roxygen: list(markdown = TRUE)
+URL: https://insightsengineering.github.io/rbmi/, https://github.com/insightsengineering/rbmi
+BugReports: https://github.com/insightsengineering/rbmi/issues
 RoxygenNote: 7.1.2
 Suggests: 
     dplyr,

diff --git a/R/mcmc.R b/R/mcmc.R
@@ -554,7 +554,7 @@ get_pattern_groups <- function(ddat) {
 #'
 #' i.e.
 #' ```
-#' patmap(c("1101", "0001"))  ->   list(c(1,2,4,0), c(4,0,0,0))
+#' patmap(c("1101", "0001"))  ->   list(c(1,2,4,999), c(4,999, 999, 999))
 #' ```
 #'
 #' @param x a character vector whose values are all either "0" or "1". All elements of
@@ -565,6 +565,10 @@ as_indices <- function(x) {
         length(unique(nchar(x))) == 1,
         msg = "all values of x must be the same length"
     )
+    assert_that(
+        unique(nchar(x)) < 999,
+        msg = "Number of pattern groups must be < 999"
+    )
 
     len <- max(nchar(x))
     lapply(
@@ -574,7 +578,7 @@ as_indices <- function(x) {
                 all(x %in% c("0", "1")),
                 msg = "All values of x must be 0 or 1"
             )
-            temp <- rep(0, len)
+            temp <- rep(999, len)
             y <- which(x == "1")
             temp[seq_along(y)] <- y
             return(temp)
@@ -633,6 +637,7 @@ validate.stan_data <- function(x, ...) {
         length(x$pat_G) == length(x$pat_sigma_index),
         length(unique(lapply(x$pat_sigma_index, length))) == 1,
         length(x$pat_sigma_index[[1]]) == x$n_visit,
-        all(vapply(x$pat_sigma_index, function(z) all(z %in% c(seq_len(x$n_visit), 0)), logical(1)))
+        all(vapply(x$pat_sigma_index, function(z) all(z %in% c(seq_len(x$n_visit), 999)), logical(1))),
+        msg = "Invalid Stan Data Object"
     )
 }
diff --git a/cran-comments.md b/cran-comments.md
@@ -1,7 +1,10 @@
 ## Resubmission
 This is a resubmission. In this version I have:
 
-* Fix a floating point issue that caused the Mac M1 CMD CHECK to fail
+* Stabilised some of our unit tests to reduce the occurrence of false-positive failures due to random variations from different CPU's/OS's.
+* Added a bug report URL and a support site URL to our DESCRIPTION file
+* Updated STAN code to hopefully fix the clang-UBSAN error. Please note though we were not able
+to reproduce this CRAN error locally so are not 100% if this fix will work.
 
 ## R CMD check results
 
@@ -17,8 +20,7 @@ There were 2 NOTEs:
 ❯ checking for GNU extensions in Makefiles ... NOTE
   GNU make is a SystemRequirements.
 
-Both of the above notes are a consequence of using rstan in the package following the usage steps as described by the stan developers [here](https://cran.r-project.org/web/packages/rstantools/vignettes/minimal-rstan-package.html). Our understanding from the [developers](https://discourse.mc-stan.org/t/using-rstan-in-an-r-package-generates-r-cmd-check-notes/26628) is that they are acceptable to ignore. 
-
+Both of the above notes are a consequence of using rstan in the package following the usage steps as described by the stan developers [here](https://cran.r-project.org/web/packages/rstantools/vignettes/minimal-rstan-package.html). Our understanding from the [developers](https://discourse.mc-stan.org/t/using-rstan-in-an-r-package-generates-r-cmd-check-notes/26628) is that they are acceptable to ignore.
 
 ## Test environments
 

diff --git a/inst/stan/MMRM.stan b/inst/stan/MMRM.stan
@@ -27,15 +27,15 @@ functions {
 
 data {
     int<lower=1> N;                         // number of observations
-    int<lower=0> P;                         // number of covariates (number of columns of design matrix)
+    int<lower=1> P;                         // number of covariates (number of columns of design matrix)
     int<lower=1> G;                         // number of Sigma Groups
     int<lower=1> n_visit;                   // number of visits
     int<lower=1> n_pat;                     // number of pat groups (# missingness patterns * groups)
 
     int<lower=1> pat_G[n_pat];              // Index for which Sigma the pat group should use
     int<lower=1> pat_n_pt[n_pat];           // number of patients in each pat group
     int<lower=1> pat_n_visit[n_pat];        // number of non-missing visits in each pat group
-    int pat_sigma_index[n_pat, n_visit];    // rows/cols from sigma to subset on for the pat group
+    int<lower=1> pat_sigma_index[n_pat, n_visit];    // rows/cols from sigma to subset on for the pat group
 
     vector[N] y;                            // outcome variable
     matrix[N,P] Q;                          // design matrix (After QR decomp)

diff --git a/man/as_indices.Rd b/man/as_indices.Rd
diff --git a/man/rbmi-package.Rd b/man/rbmi-package.Rd
diff --git a/tests/testthat/test-draws.R b/tests/testthat/test-draws.R
@@ -60,12 +60,12 @@ standard_checks <- function(dobj, d, meth) {
 test_that("approxbayes", {
 
     set.seed(40123)
-    d <- get_data(40)
-    meth <- method_approxbayes(n_samples = 6)
+    d <- get_data(80)
+    meth <- method_approxbayes(n_samples = 2)
     dobj <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
     standard_checks(dobj, d, meth)
 
-    expect_length(dobj$samples, 6)
+    expect_length(dobj$samples, 2)
     for (samp in dobj$samples) {
         expect_equal(samp$ids, levels(d$dat$id))
     }
@@ -75,12 +75,12 @@ test_that("approxbayes", {
 test_that("condmean - bootstrap", {
 
     set.seed(40123)
-    d <- get_data(40)
-    meth <- method_condmean(n_samples = 5)
+    d <- get_data(70)
+    meth <- method_condmean(n_samples = 1)
     dobj <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
     standard_checks(dobj, d, meth)
 
-    expect_length(dobj$samples, 6)
+    expect_length(dobj$samples, 2)
     expect_equal(dobj$samples[[1]]$ids, levels(d$dat$id))
 
     for (samp in dobj$samples[-1]) {
@@ -89,34 +89,47 @@ test_that("condmean - bootstrap", {
     }
 
     set.seed(623)
-    meth <- method_condmean(n_samples = 5)
+    meth <- method_condmean(n_samples = 1)
     dobj2 <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
     standard_checks(dobj2, d, meth)
 
-    expect_length(dobj2$samples, 6)
+    expect_length(dobj2$samples, 2)
     expect_equal(dobj2$samples[[1]], dobj$samples[[1]])
     expect_true(!identical(dobj$samples[[2]], dobj2$samples[[2]]))
+
+
+    set.seed(623)
+    meth <- method_condmean(n_samples = 1)
+    dobj3 <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
+    standard_checks(dobj3, d, meth)
+
+    expect_length(dobj3$samples, 2)
+    expect_equal(dobj3$samples[[1]], dobj$samples[[1]])
+    expect_true(identical(dobj2$samples, dobj3$samples))
 })
 
 
 
 test_that("condmean - jackknife", {
 
+    skip_if_not(is_nightly())
+
     set.seed(40123)
-    d <- get_data(20)
+    N <- 70
+    d <- get_data(N)
     meth <- method_condmean(type = "jackknife")
-    dobj <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
+    dobj <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE, ncores = 2)
     standard_checks(dobj, d, meth)
 
-    expect_length(dobj$samples, 21)
+    expect_length(dobj$samples, N + 1)
     expect_equal(dobj$samples[[1]]$ids, levels(d$dat$id))
-    expect_true(all(vapply(dobj$samples[-1], function(x) length(x$ids) == 19, logical(1))))
-    for (i in 1:20) {
+    expect_true(all(vapply(dobj$samples[-1], function(x) length(x$ids) == N-1, logical(1))))
+    for (i in seq_len(N)) {
         expect_equal(dobj$samples[-1][[i]]$ids, levels(d$dat$id)[-i])
     }
 
     set.seed(123)
-    dobj2 <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
+    dobj2 <- draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE, ncores = 2)
     expect_equal(dobj[c("samples", "data")], dobj2[c("samples", "data")])
 })
 
@@ -125,16 +138,15 @@ test_that("condmean - jackknife", {
 
 test_that("bayes", {
     set.seed(40123)
-    d <- get_data(40)
-    meth <- method_bayes(n_samples = 7, burn_in = 200, burn_between = 2)
+    d <- get_data(140)
+    meth <- method_bayes(n_samples = 2, burn_in = 200, burn_between = 2)
     dobj <- suppressWarnings({
         draws(d$dat, d$dat_ice, d$vars, meth, quiet = TRUE)
     })
     standard_checks(dobj, d, meth)
 
-    expect_length(dobj$samples, 7)
+    expect_length(dobj$samples, 2)
     expect_true(all(vapply(dobj$samples, function(x) all(x$ids == levels(d$dat$id)), logical(1))))
-
 })
 
 
@@ -146,14 +158,13 @@ test_that("nmar data is removed as expected", {
     # the output of draws on this dataset vs the same dataset after
     # manually removing those observations
 
-    set.seed(101)
-
+    set.seed(301)
     mysig <- as_vcov(
         sd = c(1, 3, 5),
         cor = c(0.3, 0.5, 0.8)
     )
 
-    dat <- get_sim_data(20, mysig)
+    dat <- get_sim_data(120, mysig)
 
     nmar_ids <- sample(unique(dat$id), size = 4)
 
@@ -175,8 +186,11 @@ test_that("nmar data is removed as expected", {
         covariates = c("age", "sex")
     )
 
-    method <- method_condmean(type = "jackknife")
+    method <- method_condmean(type = "bootstrap", n_samples = 2)
+
+    set.seed(101)
     d1 <- draws(dat, dat_ice, vars, method, quiet = TRUE)
+    set.seed(101)
     d2 <- draws(dat2, dat_ice, vars, method, quiet = TRUE)
     expect_equal(d1$samples, d2$samples)
 
@@ -193,7 +207,7 @@ test_that("NULL data_ice works uses MAR by default", {
 
     dobj <- draws(
         dat2,
-        method = method_condmean(n_samples = 5),
+        method = method_condmean(n_samples = 3),
         quiet = TRUE,
         vars = set_vars(
             outcome = "outcome",
@@ -214,7 +228,8 @@ test_that("NULL data_ice works uses MAR by default", {
 
 test_that("Failure is handled properly", {
 
-    bign <- 75
+    set.seed(521)
+    bign <- 80
     sigma <- as_vcov(
         c(2, 1, 0.7),
         c(
@@ -542,7 +557,7 @@ test_that("draws is calling get_mmrm_sample properly", {
 
 test_that("draws.bmlmi works as expected", {
 
-    bign <- 60
+    bign <- 120
     sigma <- as_vcov(
         c(2, 1, 0.7),
         c(
@@ -580,7 +595,7 @@ test_that("draws.bmlmi works as expected", {
         dat,
         dat_ice,
         vars,
-        method = method_bmlmi(B = 21),
+        method = method_bmlmi(B = 6),
         ncores = 2
     )
 
@@ -590,7 +605,7 @@ test_that("draws.bmlmi works as expected", {
         dat,
         dat_ice,
         vars,
-        method = method_approxbayes(n_samples = 21)
+        method = method_approxbayes(n_samples = 6)
     )
 
     ### BMLMI should be identical to approx bayes within draws except for sample ids
@@ -611,7 +626,7 @@ test_that("draws.bmlmi works as expected", {
 
 test_that("quiet supress progress messages", {
 
-    bign <- 60
+    bign <- 90
     sigma <- as_vcov(
         c(2, 1, 0.7),
         c(
@@ -649,7 +664,7 @@ test_that("quiet supress progress messages", {
             data = dat,
             data_ice = dat_ice,
             vars = vars,
-            method = method_approxbayes(n_samples = 3)
+            method = method_approxbayes(n_samples = 2)
         )
     })
     expect_true(any(grepl("Estimated running time", x)))
@@ -663,7 +678,7 @@ test_that("quiet supress progress messages", {
             quiet = TRUE,
             data_ice = dat_ice,
             vars = vars,
-            method = method_approxbayes(n_samples = 3)
+            method = method_approxbayes(n_samples = 1)
         )
     })
     expect_true(length(x) == 0 & is.character(x))