Fixed signed vs unsigned integer comparison (#281)

* Added `plot.SMCMallows()` method + example (#114) * Changed class of SMC outputs to SMCMallows (#114) This makes proper dispatching of possible, withou having any other apparent consequences. * Adapted SMC vignette and unit tests to new `plot()` method (#114) * Fixed CodeFactor issue Redundant blank line at the end of a code block should be deleted. * Gathererd SMC plot functions on same file (#114) This should help with future DRYing, perhaps both subfunctions should eventually be internalized? * Increment version number to 1.2.0.9004 * Updated NEWS.md * Updated build CI config file Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow * Added `plot.SMCMallows()` method (#263) * Added `plot.SMCMallows()` method + example (#114) * Changed class of SMC outputs to SMCMallows (#114) This makes proper dispatching of possible, withou having any other apparent consequences. * Adapted SMC vignette and unit tests to new `plot()` method (#114) * Fixed CodeFactor issue Redundant blank line at the end of a code block should be deleted. * Gathererd SMC plot functions on same file (#114) This should help with future DRYing, perhaps both subfunctions should eventually be internalized? * Increment version number to 1.2.0.9004 * Updated NEWS.md * Updated docs * Removed duplicated function As mentioned in the in-code comment, `scalefun()` was already defined in a different source file. * Increment version number to 1.2.1.9001 Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com> * Fix documentation for `plot.SMCMallows()` (#266) * Fixed links to other package functions Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it. * Increment version number to 1.2.1.9002 * Reverting changes to the `compute_mallows()` docs * Reverting changes to NEWS.md Textual change is automatically introduced by `usethis::use_dev_version()`. * Deprecated `plot_*_posterior()` (#267) Following the conversation started [here](#263 (comment)), this commit moves the superseded subfunctions of `plot.SMCMallows()` into the `smc_mallows_deprecated.R` file for eventual removal. The subfunctions themselves were renamed and test units to test the deprecation warnings were written. * Matching SMC defaults to their MCMC counterparts (#269) * Updated documentation Some text were taken from the original implementation and make no longer sense. * Matched SMC defaults with their original counterparts (#114) * SMC metric defaults to footrule (#114) * SMC leap_size defaults to 1 (#114) * SMC alpha_prop_sd defaults to 0.5 (#114) * SMC alpha_max defaults to 1e6 (#114) * SMC lambda defautls to 0.1 (#114) * Update DESCRIPTION Incremented development version. Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com> * Added changes from PR #269 to NEWS.md Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :) * Develop (#271) (#273) * Added `plot.SMCMallows()` method + example (#114) * Changed class of SMC outputs to SMCMallows (#114) This makes proper dispatching of possible, withou having any other apparent consequences. * Adapted SMC vignette and unit tests to new `plot()` method (#114) * Fixed CodeFactor issue Redundant blank line at the end of a code block should be deleted. * Gathererd SMC plot functions on same file (#114) This should help with future DRYing, perhaps both subfunctions should eventually be internalized? * Increment version number to 1.2.0.9004 * Updated NEWS.md * Updated build CI config file Using template from https://github.com/r-lib/actions/tree/v2-branch/examples#standard-ci-workflow * Added `plot.SMCMallows()` method (#263) * Added `plot.SMCMallows()` method + example (#114) * Changed class of SMC outputs to SMCMallows (#114) This makes proper dispatching of possible, withou having any other apparent consequences. * Adapted SMC vignette and unit tests to new `plot()` method (#114) * Fixed CodeFactor issue Redundant blank line at the end of a code block should be deleted. * Gathererd SMC plot functions on same file (#114) This should help with future DRYing, perhaps both subfunctions should eventually be internalized? * Increment version number to 1.2.0.9004 * Updated NEWS.md * Updated docs * Removed duplicated function As mentioned in the in-code comment, `scalefun()` was already defined in a different source file. * Increment version number to 1.2.1.9001 Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com> * Fix documentation for `plot.SMCMallows()` (#266) * Fixed links to other package functions Links were formatted wrongly assuming the package was setup to support markdown-formatted links. This fixes it. * Increment version number to 1.2.1.9002 * Reverting changes to the `compute_mallows()` docs * Reverting changes to NEWS.md Textual change is automatically introduced by `usethis::use_dev_version()`. * Deprecated `plot_*_posterior()` (#267) Following the conversation started [here](#263 (comment)), this commit moves the superseded subfunctions of `plot.SMCMallows()` into the `smc_mallows_deprecated.R` file for eventual removal. The subfunctions themselves were renamed and test units to test the deprecation warnings were written. * Matching SMC defaults to their MCMC counterparts (#269) * Updated documentation Some text were taken from the original implementation and make no longer sense. * Matched SMC defaults with their original counterparts (#114) * SMC metric defaults to footrule (#114) * SMC leap_size defaults to 1 (#114) * SMC alpha_prop_sd defaults to 0.5 (#114) * SMC alpha_max defaults to 1e6 (#114) * SMC lambda defautls to 0.1 (#114) * Update DESCRIPTION Incremented development version. Co-authored-by: Øystein Sørensen <oystein_sorensen@hotmail.com> * Added changes from PR #269 to NEWS.md Those are important to mention, since they technically break backwards compatibility. I forgot to do that on he PR itself, thankfully there's a second chance. :) Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no> Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no> * Setting Cpp version to 17 (#276) * Updated CI YAMLs, fixed lints (#278) * Updated linter CI (#270) Based on https://github.com/r-lib/actions/tree/v2/examples#lint-workflow * Removed some style rules * Fixed linting on functions and tests * Minor rewording of SMC vignette Resolved lint issues, replaced mentions of SMC-Mallows as an extension to BayesMallows (since it's now part of the package). * Updated test CI (#270) Based on https://github.com/r-lib/actions/tree/v2/examples#test-coverage-workflow * Explicitly loading package on SMC vignette Also, excludes that line from the linter check * Fixed Cpp issue (#280) --------- Co-authored-by: Waldir Leoncio <w.l.netto@medisin.uio.no>
ocbe-uio · Feb 3, 2023 · eab814c · eab814c
1 parent b2750d1
commit eab814c
Show file tree

Hide file tree

Showing 13 changed files with 58 additions and 56 deletions.
diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml
@@ -24,17 +24,18 @@ jobs:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
     steps:
         - name: Checking out the repository
-          uses: actions/checkout@v2
+          uses: actions/checkout@v3
 
         - name: Setting up R
-          uses: r-lib/actions/setup-r@v1
+          uses: r-lib/actions/setup-r@v2
           with:
             use-public-rspm: true
 
         - name: Installing dependencies
-          uses: r-lib/actions/setup-r-dependencies@v1
+          uses: r-lib/actions/setup-r-dependencies@v2
           with:
-            extra-packages: lintr
+            extra-packages: any::lintr, local::.
+            needs: lint
 
         - name: Picking on the coding style
           run: |
@@ -55,20 +56,23 @@ jobs:
               "inst/examples/sample_mallows_example.R",
               "data-raw/",
               "tests/testthat.R",
-              "R/RcppExports.R"
+              "R/RcppExports.R",
+              "vignettes/SMC-Mallows.Rmd" = 16
             )
             style_rules <- list(
               absolute_path_linter(), assignment_linter(), brace_linter(),
-              commas_linter(), commented_code_linter(), cyclocomp_linter(),
+              commas_linter(), commented_code_linter(),
               equals_na_linter(), function_left_parentheses_linter(),
-              infix_spaces_linter(), line_length_linter(), no_tab_linter(),
-              nonportable_path_linter(), object_length_linter(),
+              infix_spaces_linter(), no_tab_linter(),
+              nonportable_path_linter(),
               pipe_continuation_linter(), seq_linter(), single_quotes_linter(),
               spaces_inside_linter(), spaces_left_parentheses_linter(),
               T_and_F_symbol_linter(), todo_comment_linter(),
               trailing_blank_lines_linter(), trailing_whitespace_linter(),
-              undesirable_function_linter(), undesirable_operator_linter(),
+              undesirable_function_linter(),
               unneeded_concatenation_linter()
             )
             lint_package(linters = style_rules, exclusions = excluded_files)
           shell: Rscript {0}
+          env:
+            LINTR_ERROR_ON_LINT: true
diff --git a/.github/workflows/test-coverage.yaml b/.github/workflows/test-coverage.yaml
@@ -13,18 +13,32 @@ jobs:
     runs-on: ubuntu-latest
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
-
     steps:
-      - uses: actions/checkout@v2
-
-      - uses: r-lib/actions/setup-r@v1
+      - uses: actions/checkout@v3
+      - uses: r-lib/actions/setup-r@v2
         with:
           use-public-rspm: true
-
-      - uses: r-lib/actions/setup-r-dependencies@v1
+      - uses: r-lib/actions/setup-r-dependencies@v2
         with:
-          extra-packages: covr
-
+          extra-packages: any::covr
+          needs: coverage
       - name: Test coverage
-        run: covr::codecov()
+        run: |
+          covr::codecov(
+            quiet = FALSE,
+            clean = FALSE,
+            install_path = file.path(Sys.getenv("RUNNER_TEMP"), "package")
+          )
         shell: Rscript {0}
+      - name: Show testthat output
+        if: always()
+        run: |
+          ## --------------------------------------------------------------------
+          find ${{ runner.temp }}/package -name 'testthat.Rout*' -exec cat '{}' \; || true
+        shell: bash
+      - name: Upload test results
+        if: failure()
+        uses: actions/upload-artifact@v3
+        with:
+          name: coverage-test-failures
+          path: ${{ runner.temp }}/package
diff --git a/R/all_topological_sorts.R b/R/all_topological_sorts.R
@@ -24,4 +24,3 @@ all_topological_sorts <- function(graph, n_items, env, path = integer(),
     assign("num", get("num", envir = env) + 1, envir = env)
   }
 }
-
diff --git a/R/get_mallows_loglik.R b/R/get_mallows_loglik.R
@@ -95,7 +95,7 @@ get_mallows_loglik <- function(rho, alpha, weights, metric,
 #' @rdname get_mallows_loglik
 #' @export
 lik_db_mix <- function(rho, alpha, weights, metric,
-                       rankings, obs_freq = NULL, log = FALSE){
+                       rankings, obs_freq = NULL, log = FALSE) {
   .Deprecated(new = "get_mallows_loglik")
   get_mallows_loglik(rho, alpha, weights, metric, rankings, obs_freq, log)
 }
diff --git a/R/smc_mallows_deprecated.R b/R/smc_mallows_deprecated.R
@@ -50,7 +50,7 @@ smc_mallows_new_item_rank_alpha_fixed <- function(
   alpha, n_items, R_obs, metric, leap_size, N, Time, logz_estimate,
   mcmc_kernel_app, alpha_prop_sd, lambda, alpha_max, aug_method,
   verbose = FALSE
-){
+) {
   .Deprecated("smc_mallows_new_item_rank")
   smc_mallows_new_item_rank(
     n_items, R_obs, N, Time, logz_estimate, mcmc_kernel_app,

diff --git a/src/parameterupdates.cpp b/src/parameterupdates.cpp
@@ -13,7 +13,7 @@ arma::mat initialize_rho(int n_items, int n_cols, Rcpp::Nullable<arma::mat> rho_
     return repmat(Rcpp::as<mat>(rho_init), 1, n_cols);
   } else {
     mat rho_samples(n_items, n_cols);
-    for (uword i = 0; i < n_cols; ++i) {
+    for (int i = 0; i < n_cols; ++i) {
       rho_samples.col(i) = randperm<vec>(n_items) + 1;
     }
     return rho_samples;

diff --git a/src/smc_mallows_new_users.cpp b/src/smc_mallows_new_users.cpp
@@ -206,7 +206,7 @@ Rcpp::List smc_mallows_new_users(
                 metric, alpha_prop_sd, alpha_max, lambda\
               );
             }
-            for (uword jj = num_obs - num_new_obs; jj < num_obs; ++jj) {
+            for (int jj = num_obs - num_new_obs; jj < num_obs; ++jj) {
               rowvec ar;
               ar = aug_rankings(span(jj), span::all, span(ii));
               vec mh_aug_result;

diff --git a/tests/testthat/test-assess_convergence.R b/tests/testthat/test-assess_convergence.R
@@ -75,4 +75,3 @@ test_that("assess_convergence works with mixtures", {
   print(plt)
   dev.off()
 })
-
diff --git a/tests/testthat/test-compute_mallows.R b/tests/testthat/test-compute_mallows.R
@@ -95,7 +95,7 @@ test_that("compute_mallows handles integer preferences", {
   m <- subset(beach_preferences,
               top_item %in% c(1, 2, 3) | bottom_item %in% c(1, 2, 3))
   m[sample(nrow(m), 20), , ]
-  for(col in names(m)){
+  for (col in names(m)) {
     eval(parse(text = paste("m$", col, "<- as.integer(m$", col, ")")))
   }
 

diff --git a/tests/testthat/test-get_mallows_loglik.R b/tests/testthat/test-get_mallows_loglik.R
@@ -13,7 +13,7 @@ test_that("get_mallows_loglik works", {
   expect_equal(
     lapply(
       c("ulam", "footrule", "spearman", "kendall", "cayley", "hamming"),
-      function(m){
+      function(m) {
         get_mallows_loglik(
           rho = rbind(1:n_items, 1:n_items),
           alpha = c(2 * n_items, 2 * n_items),
@@ -31,7 +31,7 @@ test_that("get_mallows_loglik works", {
   expect_equal(
     lapply(
       c("ulam", "footrule", "spearman", "kendall", "cayley", "hamming"),
-      function(m){
+      function(m) {
         get_mallows_loglik(
           rho = 1:n_items,
           alpha = 2 * n_items,

diff --git a/tests/testthat/test-smc_individual_functions.R b/tests/testthat/test-smc_individual_functions.R
@@ -202,18 +202,18 @@ test_that("metropolis_hastings_alpha() works as expected", {
 })
 
 
-test_that("leap_and_shift_probs does not propose current ranking",{
+test_that("leap_and_shift_probs does not propose current ranking", {
   set.seed(12)
-  count <- Reduce(`+`, lapply(list(1:4, 1:10), function(rho){
-    n_items = length(rho)
+  count <- Reduce(`+`, lapply(list(1:4, 1:10), function(rho) {
+    n_items <- length(rho)
     count <- 0
-    for(i in 1:20){
+    for (i in 1:20) {
       val <- leap_and_shift_probs(rho, 1, n_items)
-      if(all(val$rho_prime == rho)) {
+      if (all(val$rho_prime == rho)) {
         count <- count + 1
       }
       val <- leap_and_shift_probs(rho, 2, n_items)
-      if(all(val$rho_prime == rho)) {
+      if (all(val$rho_prime == rho)) {
         count <- count + 1
       }
     }

diff --git a/tests/testthat/test-smc_mallows_new_item_rank.R b/tests/testthat/test-smc_mallows_new_item_rank.R
@@ -131,18 +131,18 @@ test_that("Runs with pseudo kernel", {
   expect_length(smc_unif, 4)
   expect_equal(dim(smc_unif$rho_samples), c(N, 6, 31))
   expect_equal(dim(smc_unif$alpha_samples), c(N, 31))
-  expect_equal(smc_unif$augmented_rankings[,,10],
+  expect_equal(smc_unif$augmented_rankings[, , 10],
                structure(c(1, 2, 3, 1, 3, 3, 2, 1, 1, 1, 2, 4, 2, 5, 1, 1, 3,
                            3, 2, 2, 3, 5, 4, 3, 5, 2, 5, 2, 6, 3, 4, 6, 1, 4, 6, 4, 4, 4,
                            5, 4, 6, 1, 5, 2, 4, 5, 1, 5, 3, 5, 5, 3, 6, 6, 2, 6, 6, 6, 4,
                            6), dim = c(10L, 6L)))
-  expect_equal(smc_unif$augmented_rankings[,,17],
+  expect_equal(smc_unif$augmented_rankings[, , 17],
                     structure(c(1, 2, 3, 1, 3, 3, 2, 1, 1, 1, 2, 4, 2, 5, 1, 1, 3,
                                 3, 2, 2, 3, 5, 4, 3, 5, 2, 5, 2, 6, 3, 4, 6, 1, 4, 6, 4, 4, 4,
                                 5, 4, 6, 1, 5, 2, 4, 5, 1, 5, 3, 5, 5, 3, 6, 6, 2, 6, 6, 6, 4,
                                 6), dim = c(10L, 6L)))
   expect_equal(
-    smc_unif$rho_samples[,,7],
+    smc_unif$rho_samples[, , 7],
     structure(
       c(
         4, 6, 6, 6, 3, 6, 6, 6, 6, 6, 4, 6, 6, 5, 6, 5, 5, 5, 5, 5, 5, 3, 5, 5,
@@ -155,7 +155,7 @@ test_that("Runs with pseudo kernel", {
     )
   )
   expect_equal(
-    smc_unif$alpha_samples[,10],
+    smc_unif$alpha_samples[, 10],
     c(
       0.1455621, 0.1945882, 0.0784438, 0.1732896, 0.09138699, 0.2689943,
       0.09694153, 0.1892184, 0.505403, 0.07356979, 0.7224048, 0.5333023,

diff --git a/vignettes/SMC-Mallows.Rmd b/vignettes/SMC-Mallows.Rmd
@@ -13,25 +13,12 @@ vignette: >
 
 ```{r setup, include=FALSE}
 knitr::opts_chunk$set(echo = TRUE)
-```
-
-This vignette describes the extension to the `BayesMallows` R package called `SMC-Mallows`. This uses Sequential Monte Carlo (SMC) algorithms to provide updated approximations to the posterior distribution of a single Mallows model. We consider scenarios where we receive sequential information in the form of complete rankings, partial rankings and updated rankings from existing individuals who have previously provided a (partial) ranking. We use an alternative data augmentation method, called the pseudolikelihood approach, when we are using the footrule and Spearman distance functions instead of using an independent sampler. The extension currently uses functions, similar to the `BayesMallows` R package, to visualise and analyse the posterior distributions.
-
-
-## Overview of Extension
-
-### Set-up
-
-We begin by loading the following package:
-
-```{r sushi_rankings_demo1, message=FALSE, warning=FALSE}
 library(BayesMallows)
 ```
 
+This vignette describes the `SMC-Mallows` functions of the package. These use Sequential Monte Carlo (SMC) algorithms to provide updated approximations to the posterior distribution of a single Mallows model. We consider scenarios where we receive sequential information in the form of complete rankings, partial rankings and updated rankings from existing individuals who have previously provided a (partial) ranking. We use an alternative data augmentation method, called the pseudolikelihood approach, when we are using the footrule and Spearman distance functions instead of using an independent sampler. `SMC-Mallow` uses functions similar to their base MCMC counterparts in the `BayesMallows` package, to visualise and analyse the posterior distributions.
 
-### Functions
-
-A list is provided with the most commonly used functions in this extension of the R package.
+### SMC-Mallows functions
 
 | Function Name                | Description |
 |:-----------------------------|:--------------------------------------------|
@@ -87,7 +74,7 @@ In the final stage, we move the particles using an MCMC kernel within SMC after
 
 ## SMC-Mallows User Guide
 
-The `SMC-Mallows` extension contains functions which contain algorithms to perform the Resample-Move SMC framework of @berzuini2001resample using a single Mallows model. Each algorithm begins by drawing $N$ particles using the priors for $\boldsymbol{\rho}$ and $\alpha$ or by using specified initial values. Each particle is also assigned equal weight so at the start of the SMC algorithm we have $\{\boldsymbol{\theta}^{(i)}_0 = (\boldsymbol{\rho}_0^{(i)}, \alpha_0^{(i)}), w^{(i)} \}_{i=1}^{N}$ as the set of particles. Next, we observe some ranking data, $D_t$, and we calculate the updated weights of the particles with respect to the new observations and their contribution to the current estimated posterior distribution before reweighting and multinomial resampling. Finally, we perturb the particles using the Metropolis-Hastings based MCMC kernel and we use the proposal distributions described in @vitelli2018 for sampling values of $\boldsymbol{\rho}$ and $\alpha$.
+The `SMC-Mallows` functions contain algorithms to perform the Resample-Move SMC framework of @berzuini2001resample using a single Mallows model. Each algorithm begins by drawing $N$ particles using the priors for $\boldsymbol{\rho}$ and $\alpha$ or by using specified initial values. Each particle is also assigned equal weight so at the start of the SMC algorithm we have $\{\boldsymbol{\theta}^{(i)}_0 = (\boldsymbol{\rho}_0^{(i)}, \alpha_0^{(i)}), w^{(i)} \}_{i=1}^{N}$ as the set of particles. Next, we observe some ranking data, $D_t$, and we calculate the updated weights of the particles with respect to the new observations and their contribution to the current estimated posterior distribution before reweighting and multinomial resampling. Finally, we perturb the particles using the Metropolis-Hastings based MCMC kernel and we use the proposal distributions described in @vitelli2018 for sampling values of $\boldsymbol{\rho}$ and $\alpha$.
 
 
 ### Complete Rankings
@@ -105,7 +92,7 @@ where $\alpha^{(i)}_{t-1}$ and $\boldsymbol{\rho}^{(i)}_{t-1}, \ i=1,\dots,N$ ar
 
 #### Demonstration
 
-We are interested in updating the parameter estimates of the Bayesian Mallows model based on the existing ranking data and new observations. We demonstrate the `SMC-Mallows` extension using the `sushi_rankings` dataset [@kamishima2003nantonac], which contains 5000 rankings for 10 sushi dishes.
+We are interested in updating the parameter estimates of the Bayesian Mallows model based on the existing ranking data and new observations. We demonstrate the `SMC-Mallows` functions using the `sushi_rankings` dataset [@kamishima2003nantonac], which contains 5000 rankings for 10 sushi dishes.
 
 ```{r sushi_rankings_demo}
 head(sushi_rankings)
@@ -155,7 +142,7 @@ Here, we can observe the posterior probabilities of $\boldsymbol{\rho}$ for a se
 plot(smc_test, colnames = colnames(sushi_rankings), parameter = "rho")
 ```
 
-The posterior distributions of $\boldsymbol{\rho}$ and $\alpha$ can be studied with some of the the analysis tools provided by the extension. The posterior intervals for the consensus ranking of each sushi item are obtained by calling `compute_posterior_intervals_rho`.
+The posterior distributions of $\boldsymbol{\rho}$ and $\alpha$ can be studied with some of the the analysis tools provided by `SMC-Mallows`. The posterior intervals for the consensus ranking of each sushi item are obtained by calling `compute_posterior_intervals_rho`.
 
 ```{r posterior_intervals_rho, message=FALSE, warning=FALSE}
 test_sample_rho <- smc_test$rho_samples[, , Time + 1]
@@ -248,7 +235,6 @@ set.seed(994)
 
 
 ```{r smc_partial_test}
-# aug_method = "random"
 aug_method <- "pseudolikelihood"
 metric <- "footrule"
 smc_partial_test <- smc_mallows_new_users(
Original file line number	Diff line number	Diff line change
Expand Up		@@ -24,4 +24,3 @@ all_topological_sorts <- function(graph, n_items, env, path = integer(),
		assign("num", get("num", envir = env) + 1, envir = env)
		}
		}
Original file line number	Diff line number	Diff line change
Expand Up		@@ -75,4 +75,3 @@ test_that("assess_convergence works with mixtures", {
		print(plt)
		dev.off()
		})