Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add error message for missing strata #227

Open
wants to merge 94 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
94 commits
Select commit Hold shift + click to select a range
88a24bd
add error message for missing strata
chrisorwa Aug 1, 2024
5ec8ec0
run document() for est.incidence.by
chrisorwa Aug 1, 2024
b82e3b5
add any() to is.element()
chrisorwa Aug 4, 2024
3c271ee
add test
chrisorwa Aug 7, 2024
ee3d7ca
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Aug 12, 2024
39ea75b
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Aug 14, 2024
0cb88d6
modify error messaging
chrisorwa Aug 14, 2024
97efcf3
Merge branch 'add-user-error-message-for-missing-strata' of https://g…
chrisorwa Aug 14, 2024
b3523c7
make changes to tests and est.incidence.by
chrisorwa Aug 21, 2024
c09df9b
add purrr to DESCRIPTION
chrisorwa Aug 22, 2024
42c7af6
modify error message
chrisorwa Aug 26, 2024
0f36e94
correct errors
chrisorwa Aug 26, 2024
f97218a
remove testthat from DESCRIPTION
chrisorwa Aug 26, 2024
757676e
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Aug 27, 2024
13be640
in progress
d-morrison Aug 27, 2024
87625a1
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Sep 10, 2024
3bf79b3
lint file
chrisorwa Sep 10, 2024
927911a
lint tests
chrisorwa Sep 11, 2024
655985b
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Sep 12, 2024
8302735
increment version
chrisorwa Sep 17, 2024
47dd63a
line break
d-morrison Sep 23, 2024
2ef8e8e
Merge branch 'main' into add-user-error-message-for-missing-strata
d-morrison Sep 23, 2024
6b365c0
increment version
d-morrison Sep 24, 2024
7cac80b
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Sep 29, 2024
cc000f7
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Sep 30, 2024
d3a8b8c
correct conflicts
chrisorwa Oct 13, 2024
c22ed3c
requested changes
chrisorwa Oct 13, 2024
1eb1441
push additional changes to tests
chrisorwa Oct 15, 2024
58e4598
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Oct 18, 2024
bc7fe55
Merge branch 'main' into add-user-error-message-for-missing-strata
chrisorwa Oct 22, 2024
98c881d
reduce tests
Oct 22, 2024
474d54c
Merge branch 'add-user-error-message-for-missing-strata' of https://g…
Oct 22, 2024
a2fc79a
reduce num_cores to 1
chrisorwa Oct 22, 2024
05aabc1
add snapshots
chrisorwa Oct 22, 2024
c73eb38
linting
chrisorwa Oct 22, 2024
a78d82a
correct tests
chrisorwa Oct 23, 2024
23c8ec2
Increment version number to 1.2.0.9019
chrisorwa Oct 23, 2024
5107025
more edits
d-morrison Nov 4, 2024
c0bd521
in progress
d-morrison Nov 4, 2024
3c8b8b1
lots
d-morrison Nov 5, 2024
cebb5be
more cleanup
d-morrison Nov 5, 2024
a5f4ff6
update documentation
d-morrison Nov 5, 2024
7dc8a6c
- added {and} package
d-morrison Nov 5, 2024
7cb6af9
cleanup
d-morrison Nov 5, 2024
41435d6
add `glue` and `stringr`
d-morrison Nov 5, 2024
737a1c6
add test to `check_strata()`
d-morrison Nov 5, 2024
7ba202b
remove redundant example data
d-morrison Nov 5, 2024
4232c42
wrap
d-morrison Nov 5, 2024
78af1c6
Merge branch 'simplifications' into use-test-data
d-morrison Nov 5, 2024
4b496de
Merge pull request #316 from UCD-SERG/use-test-data
d-morrison Nov 5, 2024
8478415
remove redundant files
d-morrison Nov 5, 2024
9c9626f
delete unused files
d-morrison Nov 5, 2024
28cdd72
Merge remote-tracking branch 'origin/use-test-data' into simplifications
d-morrison Nov 5, 2024
154fb5e
fixes
d-morrison Nov 5, 2024
a73c8db
Merge branch 'lint-changed-faster' into simplifications
d-morrison Nov 5, 2024
0fa2294
Merge pull request #318 from UCD-SERG/use-test-data
d-morrison Nov 5, 2024
6b362f6
Merge branch 'main' into add-user-error-message-for-missing-strata
d-morrison Nov 5, 2024
cb9c6fd
Merge branch 'add-user-error-message-for-missing-strata' into simplif…
d-morrison Nov 5, 2024
2dcff93
fix
d-morrison Nov 5, 2024
f2cce72
increase snapshot tolerance
d-morrison Nov 5, 2024
543788f
lint
d-morrison Nov 5, 2024
3682fac
document
d-morrison Nov 5, 2024
2d251c4
Merge remote-tracking branch 'origin/main' into simplifications
d-morrison Nov 5, 2024
7ad67e7
Increment version number to 1.2.0.9022
d-morrison Nov 5, 2024
d8385bb
remove duplicate line
d-morrison Nov 5, 2024
c017571
try to fix lint
d-morrison Nov 5, 2024
3ae6bed
Merge branch 'main' into add-user-error-message-for-missing-strata
d-morrison Nov 5, 2024
201853d
more linting
d-morrison Nov 5, 2024
c83e0cc
Merge branch 'add-user-error-message-for-missing-strata' into simplif…
d-morrison Nov 5, 2024
266bae1
lint
d-morrison Nov 6, 2024
c3d18bf
Merge branch 'simplifications' of https://github.com/UCD-SERG/serocal…
d-morrison Nov 6, 2024
2587245
new test to hopefully complete patch coverage
d-morrison Nov 6, 2024
c3dfcbd
skip parallel cores test if only one core available
d-morrison Nov 6, 2024
6fba6a8
added test of verbose output
d-morrison Nov 6, 2024
2e395ad
cleanup
d-morrison Nov 6, 2024
491cda6
test verbose and multicore
d-morrison Nov 6, 2024
8742288
more checks
d-morrison Nov 6, 2024
23a8baa
cleaning up time output
d-morrison Nov 6, 2024
5c98bd5
fixes
d-morrison Nov 6, 2024
1a73533
formatting
d-morrison Nov 6, 2024
ae4fc68
Merge branch 'main' into add-user-error-message-for-missing-strata
d-morrison Nov 6, 2024
d99d4da
Merge branch 'add-user-error-message-for-missing-strata' into simplif…
d-morrison Nov 6, 2024
cf0070b
Update R/est.incidence.by.R
chrisorwa Nov 6, 2024
33dc191
Update R/est.incidence.by.R
chrisorwa Nov 6, 2024
c984c18
additional changes
chrisorwa Nov 6, 2024
9122ae8
Merge branch 'add-user-error-message-for-missing-strata' of https://g…
chrisorwa Nov 6, 2024
177d334
change version number
chrisorwa Nov 6, 2024
f953b3b
make changes to NEWS.md
chrisorwa Nov 6, 2024
8dc73c8
Increment version number to 1.2.0.9022
chrisorwa Nov 6, 2024
15a32d8
Merge branch 'add-user-error-message-for-missing-strata' into simplif…
d-morrison Nov 6, 2024
15f2bc2
Merge pull request #245 from UCD-SERG/simplifications
chrisorwa Nov 6, 2024
4727731
Merge branch 'main' into add-user-error-message-for-missing-strata
d-morrison Nov 6, 2024
4e2b8e9
make changes
chrisorwa Nov 7, 2024
1cddf51
Merge branch 'add-user-error-message-for-missing-strata' of https://g…
chrisorwa Nov 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion NEWS.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# serocalculator (development version)
# serocalculator 1.3.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use dev version

Copy link
Member

@d-morrison d-morrison Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment in previous review; this change still needs to be reverted

## New features

* Add test for missing strata in `est.incidence.by` (#227)

* Added example datasets with documentation for examples and testing (#314)

* Improved error messaging for `autoplot.pop_data()` (#234).
Expand Down
130 changes: 72 additions & 58 deletions R/est.incidence.by.R
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
#' Estimate Seroincidence
#'
#' @description
#' Function to estimate seroincidences based on cross-section serology data and longitudinal
#' Function to estimate seroincidences based on cross-sectional
#' serology data and longitudinal
#' response model.
#'
#' @param pop_data a [data.frame] with cross-sectional serology data per antibody and age, and additional columns corresponding to each element of the `strata` input
#' @param strata a [character] vector of stratum-defining variables. Values must be variable names in `pop_data`.
#' @param curve_strata_varnames A subset of `strata`. Values must be variable names in `curve_params`. Default = "".
#' @param noise_strata_varnames A subset of `strata`. Values must be variable names in `noise_params`. Default = "".
#' @param num_cores Number of processor cores to use for calculations when computing by strata. If set to more than 1 and package \pkg{parallel} is available, then the computations are executed in parallel. Default = 1L.
#' @param pop_data a [data.frame] with cross-sectional serology data per
#' antibody and age, and additional columns corresponding to
#' each element of the `strata` input
#' @param strata a [character] vector of stratum-defining variables.
#' Values must be variable names in `pop_data`.
#' @param curve_strata_varnames A subset of `strata`.
#' Values must be variable names in `curve_params`. Default = "".
#' @param noise_strata_varnames A subset of `strata`.
#' Values must be variable names in `noise_params`. Default = "".
#' @param num_cores Number of processor cores to use for
#' calculations when computing by strata. If set to
#' more than 1 and package \pkg{parallel} is available,
#' then the computations are executed in parallel. Default = 1L.
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
d-morrison marked this conversation as resolved.
Show resolved Hide resolved

#' @details
#'
Expand All @@ -17,7 +25,8 @@
#' and then the data will be passed to [est.incidence()].
#' If for some reason you want to use [est.incidence.by()]
#' with no strata instead of calling [est.incidence()],
#' you may use `NA`, `NULL`, or `""` as the `strata` argument to avoid that warning.
#' you may use `NA`, `NULL`, or `""` as the `strata`
#' argument to avoid that warning.
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
#'
#'
#' @inheritParams est.incidence
Expand All @@ -26,7 +35,9 @@
#'
#' @return
#' * if `strata` has meaningful inputs:
#' An object of class `"seroincidence.by"`; i.e., a list of `"seroincidence"` objects from [est.incidence()], one for each stratum, with some meta-data attributes.
#' An object of class `"seroincidence.by"`; i.e., a list of `
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move linebreak to before the backtick

#' "seroincidence"` objects from [est.incidence()], one for each stratum,
chrisorwa marked this conversation as resolved.
Show resolved Hide resolved
#' with some meta-data attributes.
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
#' * if `strata` is missing, `NULL`, `NA`, or `""`:
#' An object of class `"seroincidence"`.
#'
Expand All @@ -39,7 +50,8 @@
#'
#' curve <- load_curve_params("https://osf.io/download/rtw5k/") %>%
#' filter(antigen_iso %in% c("HlyE_IgA", "HlyE_IgG")) %>%
#' slice(1:100, .by = antigen_iso) # Reduce dataset for the purposes of this example
#' # Reduce dataset for the purposes of this example
chrisorwa marked this conversation as resolved.
Show resolved Hide resolved
#' slice(1:100, .by = antigen_iso)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment to make the line less than 80 characters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of removing the comment, why not move it to a new line?

#'
#' noise <- load_noise_params("https://osf.io/download//hqy4v/")
#'
Expand All @@ -49,13 +61,13 @@
#' curve_params = curve,
#' noise_params = noise %>% filter(Country == "Pakistan"),
#' antigen_isos = c("HlyE_IgG", "HlyE_IgA"),
#' #num_cores = 8 # Allow for parallel processing to decrease run time
#' # num_cores = 8 # Allow for parallel processing to decrease run time
#' iterlim = 5 # limit iterations for the purpose of this example
#' )
#'
#' summary(est2)
#'
est.incidence.by <- function(

Check warning on line 70 in R/est.incidence.by.R

View workflow job for this annotation

GitHub Actions / lint-changed-files

file=R/est.incidence.by.R,line=70,col=1,[cyclocomp_linter] Functions should have cyclomatic complexity of less than 15, this has 21.

Check warning on line 70 in R/est.incidence.by.R

View workflow job for this annotation

GitHub Actions / lint-changed-files

file=R/est.incidence.by.R,line=70,col=1,[object_name_linter] Variable and function name style should match snake_case or symbols.
pop_data,
curve_params,
noise_params,
Expand All @@ -75,16 +87,19 @@
warning(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update from warning() to cli_warn()

"The `strata` argument to `est.incidence.by()` is missing.",
"\n\n If you do not want to stratify your data, ",
"consider using the `est.incidence()` function to simplify your code and avoid this warning.",
"\n\n Since the `strata` argument is empty, `est.incidence.by()` will return a `seroincidence` object, instead of a `seroincidence.by` object.\n"
"consider using the `est.incidence()` function to
simplify your code and avoid this warning.",
"\n\n Since the `strata` argument is empty, `est.incidence.by()`
will return a `seroincidence` object, instead of a
`seroincidence.by` object.\n"
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add tests everywhere indicated by Codecov comments

)
}

strata_is_empty <-
missing(strata) ||
is.null(strata) ||
setequal(strata, NA) ||
setequal(strata, "")
is.null(strata) ||
setequal(strata, NA) ||
setequal(strata, "")
d-morrison marked this conversation as resolved.
Show resolved Hide resolved

if (strata_is_empty) {
to_return <-
Expand All @@ -110,7 +125,7 @@
)

# Split data per stratum
stratumDataList <- stratify_data(

Check warning on line 128 in R/est.incidence.by.R

View workflow job for this annotation

GitHub Actions / lint-changed-files

file=R/est.incidence.by.R,line=128,col=3,[object_name_linter] Variable and function name style should match snake_case or symbols.
antigen_isos = antigen_isos,
data = pop_data %>% filter(.data$antigen_iso %in% antigen_isos),
curve_params = curve_params %>% filter(.data$antigen_iso %in% antigen_isos),
Expand All @@ -123,15 +138,17 @@
strata_table <- stratumDataList %>% attr("strata")

if (verbose) {
message("Data has been stratified.")
message("Here are the strata that will be analyzed:")
cli::cli_inform("Data has been stratified.")
cli::cli_inform("Here are the strata that will be analyzed:")

Check warning on line 142 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L141-L142

Added lines #L141 - L142 were not covered by tests
print(strata_table)
}

if (num_cores > 1L && !requireNamespace("parallel", quietly = TRUE)) {
warning(
"The `parallel` package is not installed, so `num_cores > 1` has no effect.",
"To install `parallel`, run `install.packages('parallel')` in the console."
cli::cli_warn(
"The `parallel` package is not installed,
so `num_cores > 1` has no effect.",
"To install `parallel`, run `install.packages('parallel')`
in the console."

Check warning on line 151 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L147-L151

Added lines #L147 - L151 were not covered by tests
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
)
}

Expand All @@ -142,11 +159,11 @@
num_cores <- num_cores %>% check_parallel_cores()

if (verbose) {
message("Setting up parallel processing with `num_cores` = ", num_cores, ".")
message("Setting up parallel processing with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update from message() to cli_inform()

`num_cores` = ", num_cores, ".")

Check warning on line 163 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L162-L163

Added lines #L162 - L163 were not covered by tests
d-morrison marked this conversation as resolved.
Show resolved Hide resolved
}


libPaths <- .libPaths()

Check warning on line 166 in R/est.incidence.by.R

View workflow job for this annotation

GitHub Actions / lint-changed-files

file=R/est.incidence.by.R,line=166,col=5,[object_name_linter] Variable and function name style should match snake_case or symbols.
cl <-
num_cores %>%
parallel::makeCluster() %>%
Expand All @@ -155,14 +172,19 @@
parallel::stopCluster(cl)
})

# Export library paths to the cluster
parallel::clusterExport(cl, c("libPaths"), envir = environment())

# Evaluate library loading on the cluster
parallel::clusterEvalQ(cl, {
.libPaths(libPaths)
require(serocalculator) # note - this gets out of sync when using load_all() in development
# note - this gets out of sync when using load_all() in development
require(serocalculator)

Check warning on line 182 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L182

Added line #L182 was not covered by tests
require(dplyr)
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

{
# Perform parallel computation and record execution time
time <- system.time({

Check warning on line 187 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L187

Added line #L187 was not covered by tests
fits <- parallel::parLapplyLB(
cl = cl,
X = stratumDataList,
Expand All @@ -183,49 +205,41 @@
)
}
)
} %>% system.time() -> time
})

Check warning on line 208 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L208

Added line #L208 was not covered by tests

if (verbose) {
message("Elapsed time for parallelized code: ")
print(time)
}
} else {
# fits <- lapply(
# X = stratumDataList,
# FUN = function(x) est.incidence(dataList = x, verbose = verbose, ...))

fits <- list()
fits <- list() # Initialize an empty list for fits

{ # time progress
# Time progress
for (cur_stratum in names(stratumDataList)) {
cur_stratum_vars <- strata_table %>%
dplyr::filter(.data$Stratum == cur_stratum)

for (cur_stratum in names(stratumDataList))
{
cur_stratum_vars <-
strata_table %>%
dplyr::filter(.data$Stratum == cur_stratum)

if (verbose) {
message("starting new stratum: ", cur_stratum)
print(cur_stratum_vars)
}
if (verbose) {
message("starting new stratum: ", cur_stratum)
print(cur_stratum_vars)

Check warning on line 224 in R/est.incidence.by.R

View check run for this annotation

Codecov / codecov/patch

R/est.incidence.by.R#L223-L224

Added lines #L223 - L224 were not covered by tests
}

fits[[cur_stratum]] <-
do.call(
what = est.incidence,
args = c(
stratumDataList[[cur_stratum]],
list(
lambda_start = lambda_start,
antigen_isos = antigen_isos,
build_graph = build_graph,
print_graph = print_graph,
verbose = verbose,
...
)
)
fits[[cur_stratum]] <- do.call(
what = est.incidence,
args = c(
stratumDataList[[cur_stratum]],
list(
lambda_start = lambda_start,
antigen_isos = antigen_isos,
build_graph = build_graph,
print_graph = print_graph,
verbose = verbose,
...
)
}
} %>% system.time() -> time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to lose the system.time() call; we need it below for verbose messaging.

I understand that this was changed to avoid the right-hand assignment operator ->, but there needs to be a left-hand assignment at the beginning of this whole expression instead.

)
)
}


if (verbose) {
message("Elapsed time for loop over strata: ")
Expand All @@ -233,7 +247,7 @@
}
}

incidenceData <- structure(

Check warning on line 250 in R/est.incidence.by.R

View workflow job for this annotation

GitHub Actions / lint-changed-files

file=R/est.incidence.by.R,line=250,col=3,[object_name_linter] Variable and function name style should match snake_case or symbols.
fits,
antigen_isos = antigen_isos,
Strata = strata_table,
Expand Down
32 changes: 22 additions & 10 deletions man/est.incidence.by.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions man/stratify_data.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions tests/testthat/_snaps/est.incidence.by.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# est.incidence.by() produces expected results for typhoid data

Code
typhoid_results
Output
`seroincidence.by` object estimated given the following setup:
a) Antigen isotypes : HlyE_IgG, HlyE_IgA
b) Strata : catchment

This object is a list of `seroincidence` objects, with added meta-data attributes:`antigen_isos` - Character vector of antigen isotypes used in analysis.
`Strata` - Input parameter strata of function `est.incidence.by()`

Call the `summary()` function to obtain output results.

---

structure(list("Stratum 1" = structure(list(minimum = 111.463434776159,
estimate = -2.41983536033088, gradient = -3.74675296382939e-06,
hessian = structure(15.251407603436, dim = c(1L, 1L)), code = 1L,
iterations = 4L), class = c("seroincidence", "list"), lambda_start = 0.1, antigen_isos = c("HlyE_IgG",
"HlyE_IgA")), "Stratum 2" = structure(list(minimum = 146.290052920951,
estimate = -1.640278556419, gradient = -8.66368377467952e-08,
hessian = structure(21.109488557163, dim = c(1L, 1L)), code = 1L,
iterations = 5L), class = c("seroincidence", "list"), lambda_start = 0.1, antigen_isos = c("HlyE_IgG",
"HlyE_IgA"))), antigen_isos = c("HlyE_IgG", "HlyE_IgA"), Strata = structure(list(
Stratum = c("Stratum 1", "Stratum 2"), catchment = c("aku",
"kgh"), n = c(25L, 25L)), row.names = c(NA, -2L), class = c("tbl_df",
"tbl", "data.frame"), strata_vars = "catchment"), graphs_included = FALSE, class = c("seroincidence.by",
"list"))

Loading
Loading