-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cran fixes #221
base: main
Are you sure you want to change the base?
Cran fixes #221
Conversation
Merge branch 'CRAN-fixes' of https://github.com/UCD-SERG/serocalculator into CRAN-fixes # Conflicts: # R/est.incidence.R
… into CRAN-fixes
…ission documentation
cleaned up `f_dev` documentation
NEWS.md
Outdated
|
||
# serocalculator (published versions) | ||
|
||
## serocalculator 1.0.3 | ||
* Published to CRAN | ||
|
||
## serocalculator 1.0.2 | ||
* Updated documentation and functionality based on CRAN resubmission | ||
* Deleted get_additional_data.R as it is no longer used | ||
|
||
# serocalculator (development versions) | ||
|
||
## serocalculator 1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new updates based on CRAN submission process
#' | ||
#' 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 | ||
#' autoplot() | ||
#' | ||
#' curve | ||
#' | ||
#'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added missing closing bracket
autoplot.curve_params = function( | ||
object, | ||
antigen_isos = object$antigen_iso %>% unique(), | ||
antigen_isos = unique(object$antigen_iso), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated style to avoid pipe within an argument definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me; for future reference, is this a CRAN rule, a style guide rule, or just a choice we made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this was a CRAN rule, but I haven't found it to be sure. Will update if I find it.
#' \dontrun{ | ||
#' # Estimate seroincidence | ||
#' seroincidence <- est.incidence.by(...) | ||
#' | ||
#' # Print the seroincidence object to the console | ||
#' print(seroincidence) | ||
#' \donttest{ | ||
#' | ||
#' # Or simply type (appropriate print method will be invoked automatically) | ||
#' seroincidence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed old, unfinished code and replace with an actual example
#' \dontrun{ | ||
#' # Estimate seroincidence | ||
#' seroincidence <- est.incidence.by(...) | ||
#' | ||
#' # Print the seroincidence object to the console | ||
#' print(seroincidence) | ||
#' \donttest{ | ||
#' | ||
#' # Or simply type (appropriate print method will be invoked automatically) | ||
#' seroincidence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed old, unfinished code and replace with an actual example
#' \dontrun{ | ||
#' # Estimate seroincidence | ||
#' seroincidence <- est.incidence.by(...) | ||
#' | ||
#' # Calculate summary statistics for the seroincidence object | ||
#' seroincidenceSummary <- summary(seroincidence) | ||
#' \donttest{ | ||
#' | ||
#' # Print the summary of seroincidence object to the console | ||
#' print(seroincidenceSummary) | ||
#' library(tidyverse) | ||
#' | ||
#' # Or simply type (appropriate print method will be invoked automatically) | ||
#' seroincidenceSummary | ||
#' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed old, unfinished code and replace with an actual example
#' @examples | ||
#' \dontrun{ | ||
#' expected_strata <- data.frame(Species = "banana", type = "orchid") | ||
#' | ||
#' warn.missing.strata(iris, expected_strata, dataname = "iris") | ||
#' } | ||
warn.missing.strata <- function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove example from unexported functions, per CRAN request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated section numbering format and corrected spelling errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d-morrison it doesn't look like this is an exported function but is wrapped by autoplot.curve_params.R
. Should some of the text under @details be moved to autoplot.curve_params.R
? Also since it is not longer exported, CRAN will likely have us remove the example.
\examples{ | ||
\dontrun{ | ||
expected_strata <- data.frame(Species = "banana", type = "orchid") | ||
|
||
warn.missing.strata(iris, expected_strata, dataname = "iris") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed examples from non-exported functions per CRAN request
\dontrun{ | ||
# estimate seroincidence | ||
seroincidence <- est.incidence.by(...) | ||
|
||
# calculate summary statistics for the seroincidence object | ||
seroincidenceSummary <- summary(seroincidence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed old incomplete example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with newest changes and added sections for published and development versions
#' | ||
autoplot.seroincidence = | ||
function(object, log_x = FALSE, ...) | ||
{ | ||
to_return = attr(object, "ll_graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformatted to align with other functions
#' | ||
autoplot.seroincidence.by = function( | ||
#' } | ||
autoplot.seroincidence.by <- function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to "<-" to match other functions in main branch
@@ -20,14 +20,8 @@ | |||
#' * Setting the `n_curves` argument to a value smaller than the number of rows in `curve_params` will cause this function to select the first `n_curves` rows to graph. | |||
#' * Setting `n_curves` larger than the number of rows in ` will result all curves being plotted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d-morrison since plot_curve_params_one_ab()
is not an exported function but is wrapped by autoplot.curve_params()
, should some of the text in @details be moved there?
#' @examples | ||
#' library(dplyr) # loads the `%>%` operator and `dplyr::filter()` | ||
#' | ||
#' load_curve_params("https://osf.io/download/rtw5k/") %>% | ||
#' dplyr::filter(antigen_iso == "HlyE_IgG") %>% | ||
#' serocalculator:::plot_curve_params_one_ab() | ||
#' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed example from non-exported function, per CRAN request
|
||
# ggplot() + | ||
# geom_line(data = serocourse.all, aes(x= t, y = res, group = iter)) + | ||
# facet_wrap(~antigen_iso, ncol=2) + | ||
# scale_y_log10(limits = c(0.9, 2000), breaks = c(1, 10, 100, 1000), minor_breaks = NULL) + | ||
# theme_minimal() + | ||
# theme(axis.line=element_line()) + | ||
# labs(x="Days since fever onset", y="ELISA units") | ||
|
||
# mcmc %>% ungroup() %>% slice_head(by = antigen_iso, n = 10) %>% droplevels() %>% plot_curve_params_one_ab(alpha = .4) %>% print() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed what appeared to be old code from the bottom of this file.
@d-morrison code annotation is completed and I believe I reconciled all issues between main and the CRAN branch. Since you created the PR, let me know when you think it's ready to approve. |
@kristinawlai thanks! will review asap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still reviewing; please see comments so far
@@ -1,16 +1,14 @@ | |||
Package: serocalculator | |||
Type: Package | |||
Title: Estimating Infection Rates from Serological Data | |||
Version: 1.2.0.9000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did the version number go backwards?
## serocalculator 1.0.3 | ||
* Published to CRAN | ||
|
||
## serocalculator 1.0.2 | ||
* Updated documentation and functionality based on CRAN re-submission | ||
* Deleted get_additional_data.R as it is no longer used | ||
|
||
# serocalculator (development versions) | ||
|
||
## serocalculator 1.0.3.9000 | ||
* Reconciled CRAN version 1.0.3 with GitHub main branch (version) | ||
|
||
## serocalculator 1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version numbers are out of order here; let's discuss how to reconcile this list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put release version marker where it is and leave development versions as they are. Moving forward, use CRAN version numbering (from 1.2.0.9000) --> this PR will use 1.2.0.9001
@@ -10,19 +10,21 @@ | |||
#' @return a [ggplot2::ggplot()] object | |||
#' @export | |||
#' @examples | |||
#' \donttest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve the use of \donttest
for now, but I have created #238 as a better long-term solution
samples into estimates of the frequency with which seroconversions (infections) | ||
occur in the sampled populations. Replaces the previous `seroincidence` package. | ||
Description: Translates antibody levels measured in cross-sectional population samples into estimates of the frequency with which seroconversions (infections) occur in the sampled populations. Replaces the previous 'seroincidence' package. Methods originally published in Simonsen et al. (2009) <doi:10.1002/sim.3592> and Teunis et al. (2012) <doi:10.1002/sim.5322>, and further developed in subsequent publications by de Graaf et al. (2014) <doi:10.1016/j.epidem.2014.08.002>, Teunis et al. (2016) <doi:10.1016/j.epidem.2016.04.001>, and Teunis et al. (2020) <doi:10.1002/sim.8578>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoplot.curve_params = function( | ||
object, | ||
antigen_isos = object$antigen_iso %>% unique(), | ||
antigen_isos = unique(object$antigen_iso), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me; for future reference, is this a CRAN rule, a style guide rule, or just a choice we made?
log_x = FALSE, | ||
...) | ||
{ | ||
to_return = attr(object, "ll_graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- time to learn about unit testing; please add a test to cover this line
R/autoplot.seroincidence.R
Outdated
{ | ||
to_return = attr(object, "ll_graph") | ||
#' } | ||
autoplot.seroincidence <-function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're missing a space between <-
and function
here (see https://style.tidyverse.org/syntax.html#extra-spaces:~:text=%2C%20%2D%2C-,%3C%2D,-%2C%20etc.)%20should%20always)
No description provided.