From 793db5bf34c349d644bc8a2f4c5a67b2c53e4fa8 Mon Sep 17 00:00:00 2001 From: Kathryn Doering Date: Thu, 21 Jul 2022 13:31:55 -0400 Subject: [PATCH 1/5] feat, #574: create download_models() to download the ss3 test models directory. --- NAMESPACE | 1 + R/download_models.R | 50 ++++++++++++++++++++++++ man/download_models.Rd | 46 ++++++++++++++++++++++ tests/testthat/test-download_models.R | 55 +++++++++++++++++++++++++++ 4 files changed, 152 insertions(+) create mode 100644 R/download_models.R create mode 100644 man/download_models.Rd create mode 100644 tests/testthat/test-download_models.R diff --git a/NAMESPACE b/NAMESPACE index 87ce1e177..51d05d632 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -91,6 +91,7 @@ export(SSunavailableSpawningOutput) export(TSCplot) export(add_legend) export(copy_SS_inputs) +export(download_models) export(getADMBHessian) export(get_SIS_info) export(is.wholenumber) diff --git a/R/download_models.R b/R/download_models.R new file mode 100644 index 000000000..0ad17785d --- /dev/null +++ b/R/download_models.R @@ -0,0 +1,50 @@ +#' Download SS3 test models +#' +#' Function to download the models folder within the +#' nmfs-stock-synthesis/test-models repsitory +#' @param dir The folder where the models subfolder should be written to. +#' @param branch The name of the github branch to download. +#' @template overwrite +#' @return Invisibly return a value revealing whether the files were copied +#' (TRUE) or not (FALSE). This function is used for its side effects of +#' downling SS3 test models. +#' @examples +#' download_models(dir = getwd()) +#' model_name <- list.files("models") # see the model names +#' # remove files +#' unlink(file.path("models"), recursive = TRUE) +#' @author Kathryn Doering +#' @references [nmfs-stock-synthesis/test-models repository](https://github.com/nmfs-stock-synthesis/test-models#test-models) +#' @export +download_models <- function(dir = file.path("inst", "extdata"), + branch = "main", overwrite = FALSE) { + # checks + if (!dir.exists(dir)) { + stop("dir ", dir, " does not exist") + } + zip_file_path <- file.path(dir, "test-models.zip") + result <- tryCatch(utils::download.file( + url = paste0("https://github.com/nmfs-stock-synthesis/test-models/archive/", + branch, ".zip"), + destfile = zip_file_path), + error = function(e) e) + if("simpleError" %in% class(result)) { + stop("The test models could not be downloaded. Does the branch exist?") + } + list_files <- unzip(list = TRUE, zipfile = zip_file_path) + save_files <- list_files[grep("/models/", list_files$Name, fixed = TRUE), ] + unzip(zipfile = zip_file_path, files = save_files[["Name"]], + exdir = dir) + if (dir.exists(file.path(dir, "models")) & overwrite == FALSE) { + warning("The model directory ", file.path(dir, "models"), " already exists ", + "\nand overwrite is FALSE, so no new files will be written." ) + } + dir.create(file.path(dir, "models"), showWarnings = FALSE) + copy_status <- file.copy(from = file.path(dir, paste0("test-models-", branch), "models"), + to = file.path(dir), recursive = TRUE, overwrite = overwrite) + # clean up + unlink(zip_file_path) + unlink(file.path(dir, paste0("test-models-", branch)), + recursive = TRUE) + invisible(copy_status) + } \ No newline at end of file diff --git a/man/download_models.Rd b/man/download_models.Rd new file mode 100644 index 000000000..8b4e5c549 --- /dev/null +++ b/man/download_models.Rd @@ -0,0 +1,46 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/download_models.R +\name{download_models} +\alias{download_models} +\title{Download SS3 test models + +Function to download the models folder within the +nmfs-stock-synthesis/test-models repsitory} +\usage{ +download_models( + dir = file.path("inst", "extdata"), + branch = "main", + overwrite = FALSE +) +} +\arguments{ +\item{dir}{The folder where the models subfolder should be written to.} + +\item{branch}{The name of the github branch to download.} + +\item{overwrite}{A logical value specifying if the existing file(s) +should be overwritten. The default value is \code{overwrite = FALSE}.} +} +\value{ +Invisibly return a value revealing whether the files were copied +(TRUE) or not (FALSE). This function is used for its side effects of +downling SS3 test models. +} +\description{ +Download SS3 test models + +Function to download the models folder within the +nmfs-stock-synthesis/test-models repsitory +} +\examples{ + download_models(dir = getwd()) + model_name <- list.files("models") # see the model names + # remove files + unlink(file.path("models"), recursive = TRUE) +} +\references{ +\href{https://github.com/nmfs-stock-synthesis/test-models#test-models}{nmfs-stock-synthesis/test-models repository} +} +\author{ +Kathryn Doering +} diff --git a/tests/testthat/test-download_models.R b/tests/testthat/test-download_models.R new file mode 100644 index 000000000..73b520190 --- /dev/null +++ b/tests/testthat/test-download_models.R @@ -0,0 +1,55 @@ + +# setup +temp_dir <- tempdir() +save_models_dir <- file.path(temp_dir, "save_mods") +# create directories and use on exit statements to ensure cleanup at the end. +dir.create(save_models_dir) +on.exit(unlink(save_models_dir, recursive = TRUE), add = TRUE) +if (!dir.exists("inst")) { + dir.create("inst") + dir.create("inst/extdata") + on.exit(unlink("inst", recursive = TRUE), add = TRUE) +} +if (!dir.exists("inst/extdata")) { + dir.create("inst/extdata") + on.exit(unlink("inst/extdata", recursive = TRUE), add = TRUE) +} + +test_that("download_models with defaults work", { + download_models() + expect_true(file.exists("inst/extdata/models")) + expect_true(file.exists("inst/extdata/models/Simple")) + expect_true(file.exists("inst/extdata/models/Simple/ss.par")) +}) + +test_that("download_models with different dir works", { + download_models(dir = save_models_dir) + expect_true(file.exists(file.path(save_models_dir, "models"))) + expect_true(file.exists(file.path(save_models_dir, "models", "Simple"))) + expect_true(file.exists(file.path(save_models_dir, "models", "Simple", "ss.par"))) +}) + +test_that("download models works with different branch", { + temp_dir_branch <- file.path(save_models_dir, "diff_branch") + dir.create(temp_dir_branch) + download_models(dir = temp_dir_branch, branch = "super_test") + expect_true(file.exists(file.path(temp_dir_branch, "models"))) + expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple"))) + expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple", "ss.par"))) +}) + +test_that("download_models() fails when the branch doesn't exist", { + temp_dir_branch_dne <- file.path(save_models_dir, "diff_branch_dne") + dir.create(temp_dir_branch_dne) + expect_error( + download_models(dir = temp_dir_branch_dne, + branch = "not_existing_branch"), + "The test models could not be downloaded") +}) + +test_that("download_models() fails when the dir doesn't exist", { + temp_dir_missing_dir <- file.path(save_models_dir, "missing") + # don't create it + expect_error(download_models(dir = temp_dir_missing_dir), "does not exist") + +}) \ No newline at end of file From e1c829c9cf784f52230b4be09ca98fe0adc71cf7 Mon Sep 17 00:00:00 2001 From: Kathryn Doering Date: Thu, 21 Jul 2022 16:23:48 -0400 Subject: [PATCH 2/5] add utils:: in front of instances of unzip to rm warning --- R/download_models.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/download_models.R b/R/download_models.R index 0ad17785d..4c8d748c6 100644 --- a/R/download_models.R +++ b/R/download_models.R @@ -31,9 +31,9 @@ download_models <- function(dir = file.path("inst", "extdata"), if("simpleError" %in% class(result)) { stop("The test models could not be downloaded. Does the branch exist?") } - list_files <- unzip(list = TRUE, zipfile = zip_file_path) + list_files <- utils::unzip(list = TRUE, zipfile = zip_file_path) save_files <- list_files[grep("/models/", list_files$Name, fixed = TRUE), ] - unzip(zipfile = zip_file_path, files = save_files[["Name"]], + utils::unzip(zipfile = zip_file_path, files = save_files[["Name"]], exdir = dir) if (dir.exists(file.path(dir, "models")) & overwrite == FALSE) { warning("The model directory ", file.path(dir, "models"), " already exists ", From 424e88d6048a9f1e291527b7b90be896a8c22587 Mon Sep 17 00:00:00 2001 From: Kathryn Doering Date: Thu, 21 Jul 2022 16:32:49 -0400 Subject: [PATCH 3/5] fix: comment out a test This test would fail if the super_test feature branch is removed from the test-models repo, so it seems wise to not include it. However, it was a nice local test to make sure the function worked as I expected when setting it up! --- tests/testthat/test-download_models.R | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/testthat/test-download_models.R b/tests/testthat/test-download_models.R index 73b520190..635b0ca66 100644 --- a/tests/testthat/test-download_models.R +++ b/tests/testthat/test-download_models.R @@ -29,14 +29,14 @@ test_that("download_models with different dir works", { expect_true(file.exists(file.path(save_models_dir, "models", "Simple", "ss.par"))) }) -test_that("download models works with different branch", { - temp_dir_branch <- file.path(save_models_dir, "diff_branch") - dir.create(temp_dir_branch) - download_models(dir = temp_dir_branch, branch = "super_test") - expect_true(file.exists(file.path(temp_dir_branch, "models"))) - expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple"))) - expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple", "ss.par"))) -}) +# test_that("download models works with different branch", { +# temp_dir_branch <- file.path(save_models_dir, "diff_branch") +# dir.create(temp_dir_branch) +# download_models(dir = temp_dir_branch, branch = "super_test") +# expect_true(file.exists(file.path(temp_dir_branch, "models"))) +# expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple"))) +# expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple", "ss.par"))) +# }) test_that("download_models() fails when the branch doesn't exist", { temp_dir_branch_dne <- file.path(save_models_dir, "diff_branch_dne") From 2566798e544afea6b86077126b0667061d22edc1 Mon Sep 17 00:00:00 2001 From: Kathryn Doering Date: Fri, 22 Jul 2022 13:56:37 -0400 Subject: [PATCH 4/5] fix: documentation wording, create folders, changes tests suggestions from @kellijohnson-NOAA in PR #721 --- R/download_models.R | 19 +++++++++++-------- tests/testthat/test-download_models.R | 27 +++++++++++---------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/R/download_models.R b/R/download_models.R index 4c8d748c6..be2e5757d 100644 --- a/R/download_models.R +++ b/R/download_models.R @@ -1,11 +1,14 @@ #' Download SS3 test models #' -#' Function to download the models folder within the -#' nmfs-stock-synthesis/test-models repsitory -#' @param dir The folder where the models subfolder should be written to. -#' @param branch The name of the github branch to download. +#' Download and unzip the models folder stored on GitHub within the +#' nmfs-stock-synthesis/test-models repository. +#' @param dir A file path where the downloaded `"models"` subfolder will be written to. +#' @param branch A string specifying the desired branch of +#' [nmfs-stock-synthesis/test-models repository](https://github.com/nmfs-stock-synthesis/test-models#test-models) +#' that you want to download. The default is `"main"`, which is the +#' stable/default branch. #' @template overwrite -#' @return Invisibly return a value revealing whether the files were copied +#' @return Invisibly return a logical revealing whether the files were copied #' (TRUE) or not (FALSE). This function is used for its side effects of #' downling SS3 test models. #' @examples @@ -20,7 +23,7 @@ download_models <- function(dir = file.path("inst", "extdata"), branch = "main", overwrite = FALSE) { # checks if (!dir.exists(dir)) { - stop("dir ", dir, " does not exist") + dir.create(dir, recursive = TRUE) } zip_file_path <- file.path(dir, "test-models.zip") result <- tryCatch(utils::download.file( @@ -37,7 +40,7 @@ download_models <- function(dir = file.path("inst", "extdata"), exdir = dir) if (dir.exists(file.path(dir, "models")) & overwrite == FALSE) { warning("The model directory ", file.path(dir, "models"), " already exists ", - "\nand overwrite is FALSE, so no new files will be written." ) + "\nand overwrite is FALSE. So, no new files will be written.") } dir.create(file.path(dir, "models"), showWarnings = FALSE) copy_status <- file.copy(from = file.path(dir, paste0("test-models-", branch), "models"), @@ -47,4 +50,4 @@ download_models <- function(dir = file.path("inst", "extdata"), unlink(file.path(dir, paste0("test-models-", branch)), recursive = TRUE) invisible(copy_status) - } \ No newline at end of file + } diff --git a/tests/testthat/test-download_models.R b/tests/testthat/test-download_models.R index 635b0ca66..6f90f913d 100644 --- a/tests/testthat/test-download_models.R +++ b/tests/testthat/test-download_models.R @@ -29,27 +29,22 @@ test_that("download_models with different dir works", { expect_true(file.exists(file.path(save_models_dir, "models", "Simple", "ss.par"))) }) -# test_that("download models works with different branch", { -# temp_dir_branch <- file.path(save_models_dir, "diff_branch") -# dir.create(temp_dir_branch) -# download_models(dir = temp_dir_branch, branch = "super_test") -# expect_true(file.exists(file.path(temp_dir_branch, "models"))) -# expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple"))) -# expect_true(file.exists(file.path(temp_dir_branch, "models", "Simple", "ss.par"))) -# }) +test_that("download_models() works when the dir doesn't exist", { + temp_dir_missing_dir <- file.path(save_models_dir, "missing_dir", "missing_subdir") + # create it + worked <- download_models(dir = temp_dir_missing_dir) + expect_true(worked) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models"))) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple"))) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple", "ss.par"))) +}) +# (note that this generates 2 warnings, and I'm not sure how to suppress them!) test_that("download_models() fails when the branch doesn't exist", { temp_dir_branch_dne <- file.path(save_models_dir, "diff_branch_dne") dir.create(temp_dir_branch_dne) expect_error( download_models(dir = temp_dir_branch_dne, branch = "not_existing_branch"), - "The test models could not be downloaded") + "not be downloaded") }) - -test_that("download_models() fails when the dir doesn't exist", { - temp_dir_missing_dir <- file.path(save_models_dir, "missing") - # don't create it - expect_error(download_models(dir = temp_dir_missing_dir), "does not exist") - -}) \ No newline at end of file From f409bae83a29bf293156d00749be039d7fa200ad Mon Sep 17 00:00:00 2001 From: Kathryn Doering Date: Wed, 27 Jul 2022 12:44:46 -0400 Subject: [PATCH 5/5] fix: more changes proposed by @kellijohnson-NOAA including running devtools::spell_check() devtools::document() and styler::style_file() for the files related to download_models --- R/download_models.R | 71 +++++++++++++++++---------- man/download_models.Rd | 30 ++++++----- tests/testthat/test-download_models.R | 49 +++++++++--------- 3 files changed, 84 insertions(+), 66 deletions(-) diff --git a/R/download_models.R b/R/download_models.R index be2e5757d..5afb90525 100644 --- a/R/download_models.R +++ b/R/download_models.R @@ -1,53 +1,70 @@ #' Download SS3 test models -#' -#' Download and unzip the models folder stored on GitHub within the +#' +#' Download and unzip the models folder stored on GitHub within the #' nmfs-stock-synthesis/test-models repository. #' @param dir A file path where the downloaded `"models"` subfolder will be written to. #' @param branch A string specifying the desired branch of #' [nmfs-stock-synthesis/test-models repository](https://github.com/nmfs-stock-synthesis/test-models#test-models) -#' that you want to download. The default is `"main"`, which is the +#' that you want to download. The default is `"main"`, which is the #' stable/default branch. #' @template overwrite -#' @return Invisibly return a logical revealing whether the files were copied +#' @return Invisibly return a logical revealing whether the files were copied #' (TRUE) or not (FALSE). This function is used for its side effects of -#' downling SS3 test models. +#' downloading SS3 test models. #' @examples -#' download_models(dir = getwd()) -#' model_name <- list.files("models") # see the model names -#' # remove files -#' unlink(file.path("models"), recursive = TRUE) +#' download_models(dir = getwd()) +#' model_name <- list.files("models") # see the model names +#' # remove files +#' unlink(file.path("models"), recursive = TRUE) #' @author Kathryn Doering #' @references [nmfs-stock-synthesis/test-models repository](https://github.com/nmfs-stock-synthesis/test-models#test-models) #' @export -download_models <- function(dir = file.path("inst", "extdata"), - branch = "main", overwrite = FALSE) { +download_models <- function(dir = file.path("inst", "extdata"), + branch = "main", + overwrite = FALSE) { # checks if (!dir.exists(dir)) { dir.create(dir, recursive = TRUE) } zip_file_path <- file.path(dir, "test-models.zip") - result <- tryCatch(utils::download.file( - url = paste0("https://github.com/nmfs-stock-synthesis/test-models/archive/", - branch, ".zip"), - destfile = zip_file_path), - error = function(e) e) - if("simpleError" %in% class(result)) { - stop("The test models could not be downloaded. Does the branch exist?") - } + result <- tryCatch( + utils::download.file( + url = paste0( + "https://github.com/nmfs-stock-synthesis/test-models/archive/", + branch, + ".zip" + ), + destfile = zip_file_path + ), + error = function(e) { + stop( + "The test-models zip file could not be downloaded.", + " Does the branch (", branch, ") exist?", + call. = FALSE + ) + } + ) list_files <- utils::unzip(list = TRUE, zipfile = zip_file_path) save_files <- list_files[grep("/models/", list_files$Name, fixed = TRUE), ] - utils::unzip(zipfile = zip_file_path, files = save_files[["Name"]], - exdir = dir) + utils::unzip( + zipfile = zip_file_path, files = save_files[["Name"]], + exdir = dir + ) if (dir.exists(file.path(dir, "models")) & overwrite == FALSE) { - warning("The model directory ", file.path(dir, "models"), " already exists ", - "\nand overwrite is FALSE. So, no new files will be written.") + warning( + "The model directory ", file.path(dir, "models"), " already exists ", + "\nand overwrite is FALSE. So, no new files will be written." + ) } dir.create(file.path(dir, "models"), showWarnings = FALSE) - copy_status <- file.copy(from = file.path(dir, paste0("test-models-", branch), "models"), - to = file.path(dir), recursive = TRUE, overwrite = overwrite) + copy_status <- file.copy( + from = file.path(dir, paste0("test-models-", branch), "models"), + to = file.path(dir), recursive = TRUE, overwrite = overwrite + ) # clean up unlink(zip_file_path) unlink(file.path(dir, paste0("test-models-", branch)), - recursive = TRUE) + recursive = TRUE + ) invisible(copy_status) - } +} diff --git a/man/download_models.Rd b/man/download_models.Rd index 8b4e5c549..714ec5e09 100644 --- a/man/download_models.Rd +++ b/man/download_models.Rd @@ -2,10 +2,7 @@ % Please edit documentation in R/download_models.R \name{download_models} \alias{download_models} -\title{Download SS3 test models - -Function to download the models folder within the -nmfs-stock-synthesis/test-models repsitory} +\title{Download SS3 test models} \usage{ download_models( dir = file.path("inst", "extdata"), @@ -14,29 +11,30 @@ download_models( ) } \arguments{ -\item{dir}{The folder where the models subfolder should be written to.} +\item{dir}{A file path where the downloaded \code{"models"} subfolder will be written to.} -\item{branch}{The name of the github branch to download.} +\item{branch}{A string specifying the desired branch of +\href{https://github.com/nmfs-stock-synthesis/test-models#test-models}{nmfs-stock-synthesis/test-models repository} +that you want to download. The default is \code{"main"}, which is the +stable/default branch.} \item{overwrite}{A logical value specifying if the existing file(s) should be overwritten. The default value is \code{overwrite = FALSE}.} } \value{ -Invisibly return a value revealing whether the files were copied +Invisibly return a logical revealing whether the files were copied (TRUE) or not (FALSE). This function is used for its side effects of -downling SS3 test models. +downloading SS3 test models. } \description{ -Download SS3 test models - -Function to download the models folder within the -nmfs-stock-synthesis/test-models repsitory +Download and unzip the models folder stored on GitHub within the +nmfs-stock-synthesis/test-models repository. } \examples{ - download_models(dir = getwd()) - model_name <- list.files("models") # see the model names - # remove files - unlink(file.path("models"), recursive = TRUE) +download_models(dir = getwd()) +model_name <- list.files("models") # see the model names +# remove files +unlink(file.path("models"), recursive = TRUE) } \references{ \href{https://github.com/nmfs-stock-synthesis/test-models#test-models}{nmfs-stock-synthesis/test-models repository} diff --git a/tests/testthat/test-download_models.R b/tests/testthat/test-download_models.R index 6f90f913d..623f1564f 100644 --- a/tests/testthat/test-download_models.R +++ b/tests/testthat/test-download_models.R @@ -11,40 +11,43 @@ if (!dir.exists("inst")) { on.exit(unlink("inst", recursive = TRUE), add = TRUE) } if (!dir.exists("inst/extdata")) { - dir.create("inst/extdata") - on.exit(unlink("inst/extdata", recursive = TRUE), add = TRUE) + dir.create("inst/extdata") + on.exit(unlink("inst/extdata", recursive = TRUE), add = TRUE) } test_that("download_models with defaults work", { - download_models() - expect_true(file.exists("inst/extdata/models")) - expect_true(file.exists("inst/extdata/models/Simple")) - expect_true(file.exists("inst/extdata/models/Simple/ss.par")) + download_models() + expect_true(file.exists("inst/extdata/models")) + expect_true(file.exists("inst/extdata/models/Simple")) + expect_true(file.exists("inst/extdata/models/Simple/ss.par")) }) test_that("download_models with different dir works", { - download_models(dir = save_models_dir) - expect_true(file.exists(file.path(save_models_dir, "models"))) - expect_true(file.exists(file.path(save_models_dir, "models", "Simple"))) - expect_true(file.exists(file.path(save_models_dir, "models", "Simple", "ss.par"))) + download_models(dir = save_models_dir) + expect_true(file.exists(file.path(save_models_dir, "models"))) + expect_true(file.exists(file.path(save_models_dir, "models", "Simple"))) + expect_true(file.exists(file.path(save_models_dir, "models", "Simple", "ss.par"))) }) test_that("download_models() works when the dir doesn't exist", { - temp_dir_missing_dir <- file.path(save_models_dir, "missing_dir", "missing_subdir") - # create it - worked <- download_models(dir = temp_dir_missing_dir) - expect_true(worked) - expect_true(file.exists(file.path(temp_dir_missing_dir, "models"))) - expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple"))) - expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple", "ss.par"))) + temp_dir_missing_dir <- file.path(save_models_dir, "missing_dir", "missing_subdir") + # create it + worked <- download_models(dir = temp_dir_missing_dir) + expect_true(worked) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models"))) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple"))) + expect_true(file.exists(file.path(temp_dir_missing_dir, "models", "Simple", "ss.par"))) }) # (note that this generates 2 warnings, and I'm not sure how to suppress them!) test_that("download_models() fails when the branch doesn't exist", { - temp_dir_branch_dne <- file.path(save_models_dir, "diff_branch_dne") - dir.create(temp_dir_branch_dne) - expect_error( - download_models(dir = temp_dir_branch_dne, - branch = "not_existing_branch"), - "not be downloaded") + temp_dir_branch_dne <- file.path(save_models_dir, "diff_branch_dne") + dir.create(temp_dir_branch_dne) + expect_error( + download_models( + dir = temp_dir_branch_dne, + branch = "not_existing_branch" + ), + "not be downloaded" + ) })