Conversation
Merge commit '4ee4b99f9f601b2f95fab79fb25a7d781b37e1a7' #Conflicts: # R/Run_Mod.R
Codecov Report❌ Patch coverage is
|
|
📖 https://ucd-serg.github.io/serodynamics/preview/pr114 |
|
I am a little confused with the warning/error I am getting here. It looks like there is a scoping issue relating to the base dataset when using dplyr, but when I fix the issue the tests are producing a warning. It seems like the solution might be in conflicting with each other. Grateful for any help! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| print.sr_model <- function(x, | ||
| print_tbl = FALSE, | ||
| ...) { # nolint | ||
| if (print_tbl) { | ||
| x <- dplyr::as_tibble(x) | ||
| print(x) | ||
| invisible(x) |
There was a problem hiding this comment.
The ... argument is accepted but not used. In S3 print methods, ... should typically be forwarded to the underlying print() call (e.g., to control digits, n, width, etc.); otherwise remove ... from the signature/docs.
| #' predictive posterior distribution for antibody kinetic curve parameters | ||
| #' by `Iso_type` and `Stratification` (if specified). | ||
| #' @param x An `sr_model` output object from [run_mod()]. | ||
| #' @param print_tbl A [logical] indicator to print in style of [dplyr::tbl_df]. | ||
| #' @param ... Additional arguments affecting the summary produced. | ||
| #' [serodynamics::run_mod()] function. | ||
| #' @returns A data summary that | ||
| #' contains the median posterior distribution for antibody kinetic curve | ||
| #' parameters by `Iso_type` and `Stratification` (if specified). | ||
| #' @export | ||
| #' @examples | ||
| #' print(nepal_sees_jags_output) | ||
| print.sr_model <- function(x, | ||
| print_tbl = FALSE, | ||
| ...) { # nolint | ||
| if (print_tbl) { | ||
| x <- dplyr::as_tibble(x) | ||
| print(x) | ||
| invisible(x) | ||
| } else { | ||
| cat("An sr_model with the following median values:") | ||
| cat("\n") | ||
| cat("\n") | ||
| x <- suppressWarnings({ | ||
| x |> | ||
| dplyr::filter(.data$Subject == "newperson") |> | ||
| dplyr::summarise(.by = c(.data$Stratification, .data$Iso_type, | ||
| .data$Parameter), | ||
| median_val = stats::median(.data$value)) |> | ||
| tidyr::pivot_wider(names_from = .data$Parameter, | ||
| values_from = .data$median_val) |> | ||
| dplyr::arrange(.data$Iso_type) | ||
| }) | ||
| # Taking out stratification column if not specified | ||
| if (!"Stratification" %in% names(x) || all(x$Stratification == "None", | ||
| na.rm = TRUE)) { | ||
| x <- dplyr::select(x, -dplyr::any_of("Stratification")) | ||
| } | ||
| print(as.data.frame(x)) | ||
| invisible(x) | ||
| } |
There was a problem hiding this comment.
This print.sr_model() returns the computed summary tibble/data.frame invisibly rather than returning the original sr_model object. That’s non-standard for print() methods and can surprise callers who do y <- print(x) and expect y to still be an sr_model. Consider returning the original x invisibly and (if you want a reusable summary) expose a separate helper (e.g., summarise_sr_model()/median_sr_model()) to return the table.
| #' predictive posterior distribution for antibody kinetic curve parameters | |
| #' by `Iso_type` and `Stratification` (if specified). | |
| #' @param x An `sr_model` output object from [run_mod()]. | |
| #' @param print_tbl A [logical] indicator to print in style of [dplyr::tbl_df]. | |
| #' @param ... Additional arguments affecting the summary produced. | |
| #' [serodynamics::run_mod()] function. | |
| #' @returns A data summary that | |
| #' contains the median posterior distribution for antibody kinetic curve | |
| #' parameters by `Iso_type` and `Stratification` (if specified). | |
| #' @export | |
| #' @examples | |
| #' print(nepal_sees_jags_output) | |
| print.sr_model <- function(x, | |
| print_tbl = FALSE, | |
| ...) { # nolint | |
| if (print_tbl) { | |
| x <- dplyr::as_tibble(x) | |
| print(x) | |
| invisible(x) | |
| } else { | |
| cat("An sr_model with the following median values:") | |
| cat("\n") | |
| cat("\n") | |
| x <- suppressWarnings({ | |
| x |> | |
| dplyr::filter(.data$Subject == "newperson") |> | |
| dplyr::summarise(.by = c(.data$Stratification, .data$Iso_type, | |
| .data$Parameter), | |
| median_val = stats::median(.data$value)) |> | |
| tidyr::pivot_wider(names_from = .data$Parameter, | |
| values_from = .data$median_val) |> | |
| dplyr::arrange(.data$Iso_type) | |
| }) | |
| # Taking out stratification column if not specified | |
| if (!"Stratification" %in% names(x) || all(x$Stratification == "None", | |
| na.rm = TRUE)) { | |
| x <- dplyr::select(x, -dplyr::any_of("Stratification")) | |
| } | |
| print(as.data.frame(x)) | |
| invisible(x) | |
| } | |
| #' predictive posterior distribution for antibody kinetic curve parameters | |
| #' by `Iso_type` and `Stratification` (if specified). | |
| #' @param x An `sr_model` output object from [run_mod()]. | |
| #' @param print_tbl A [logical] indicator to print in style of [dplyr::tbl_df]. | |
| #' @param ... Additional arguments affecting the summary produced. | |
| #' [serodynamics::run_mod()] function. | |
| #' @returns The original `sr_model` object, returned invisibly. | |
| #' @export | |
| #' @examples | |
| #' print(nepal_sees_jags_output) | |
| print.sr_model <- function(x, | |
| print_tbl = FALSE, | |
| ...) { # nolint | |
| if (print_tbl) { | |
| print_data <- dplyr::as_tibble(x) | |
| print(print_data) | |
| invisible(x) | |
| } else { | |
| cat("An sr_model with the following median values:") | |
| cat("\n") | |
| cat("\n") | |
| summary_tbl <- suppressWarnings({ | |
| x |> | |
| dplyr::filter(.data$Subject == "newperson") |> | |
| dplyr::summarise(.by = c(.data$Stratification, .data$Iso_type, | |
| .data$Parameter), | |
| median_val = stats::median(.data$value)) |> | |
| tidyr::pivot_wider(names_from = .data$Parameter, | |
| values_from = .data$median_val) |> | |
| dplyr::arrange(.data$Iso_type) | |
| }) | |
| # Taking out stratification column if not specified | |
| if (!"Stratification" %in% names(summary_tbl) || | |
| all(summary_tbl$Stratification == "None", na.rm = TRUE)) { | |
| summary_tbl <- dplyr::select(summary_tbl, | |
| -dplyr::any_of("Stratification")) | |
| } | |
| print(as.data.frame(summary_tbl)) | |
| invisible(x) | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot please provide suggestions regarding how to fix the documentation error that I am receiving in the review. It looks like some Rd. files have been changed but when I try to run devtools::document() in the R interface there are no recommended updates. |
Agent-Logs-Url: https://github.com/ucdavis/serodynamics/sessions/9e5d11ac-b621-49bc-9420-165105d24b90 Co-authored-by: sschildhauer <165851188+sschildhauer@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ucdavis/serodynamics/sessions/9e5d11ac-b621-49bc-9420-165105d24b90 Co-authored-by: sschildhauer <165851188+sschildhauer@users.noreply.github.com>
Fixed in commit
Fixed in commit |
Kwan-Jenny
left a comment
There was a problem hiding this comment.
Overall looks good and the tidyselect fix is clean! Just a couple of small things I noticed:
-
In
print.sr_model, the whole pipe is wrapped insuppressWarnings()— would be helpful to have a quick comment on what warning is being suppressed, just so it's easier to debug later if something breaks silently. -
Is
"None"a sentinel value coming from the model output? If so, maybe worth a short comment explaining where it comes from so it doesn't look like a magic string. -
In the test file,
suppressWarnings()is used inside the third test too — same thing, might be worth being a bit more explicit about what's expected there.
Minor stuff overall, nothing blocking.
Three CI failures in the
print.sr_modelPR: tidyselect deprecation warnings from.datain non-data-masking contexts, Rd files out of sync with source, and a missing snapshot for the unstratified test case.Changes
Fix
.datain tidyselect contexts —.by,names_from, andvalues_fromare tidyselect args, not data-masking; replaced with string column names:Regenerate documentation — ran
devtools::document()to syncman/print.sr_model.Rdand four other Rd files that had drifted from source; also fixed[dplyr::tbl_df]→[tibble::tbl_df]in theprint_tblparam doc.Add missing snapshot — added the Linux/non-Darwin snapshot for the unstratified
sr_modelprint test (the macOS variant will be generated by CI on the first passing run).