Skip to content

Creating a default print for sr_model#114

Open
sschildhauer wants to merge 105 commits intomainfrom
default_print
Open

Creating a default print for sr_model#114
sschildhauer wants to merge 105 commits intomainfrom
default_print

Conversation

@sschildhauer
Copy link
Copy Markdown
Collaborator

@sschildhauer sschildhauer commented Jul 9, 2025

Three CI failures in the print.sr_model PR: tidyselect deprecation warnings from .data in non-data-masking contexts, Rd files out of sync with source, and a missing snapshot for the unstratified test case.

Changes

  • Fix .data in tidyselect contexts.by, names_from, and values_from are tidyselect args, not data-masking; replaced with string column names:

    # Before
    dplyr::summarise(.by = c(.data$Stratification, .data$Iso_type, .data$Parameter), ...)
    tidyr::pivot_wider(names_from = .data$Parameter, values_from = .data$median_val)
    
    # After
    dplyr::summarise(.by = c("Stratification", "Iso_type", "Parameter"), ...)
    tidyr::pivot_wider(names_from = "Parameter", values_from = "median_val")
  • Regenerate documentation — ran devtools::document() to sync man/print.sr_model.Rd and four other Rd files that had drifted from source; also fixed [dplyr::tbl_df][tibble::tbl_df] in the print_tbl param doc.

  • Add missing snapshot — added the Linux/non-Darwin snapshot for the unstratified sr_model print test (the macOS variant will be generated by CI on the first passing run).

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 9, 2025

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
R/print.sr_model.R 94.73% 1 Missing ⚠️
Files with missing lines Coverage Δ
R/Run_Mod.R 100.00% <ø> (ø)
R/expect_snapshot_data.R 100.00% <ø> (ø)
R/print.sr_model.R 94.73% <94.73%> (ø)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 9, 2025

📖 https://ucd-serg.github.io/serodynamics/preview/pr114
Preview documentation for this PR (at commit 0be057f)

@sschildhauer
Copy link
Copy Markdown
Collaborator Author

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!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Comment thread R/print.sr_model.R Outdated
Comment thread tests/testthat/test-print.sr_model.R Outdated
Copilot AI review requested due to automatic review settings April 8, 2026 09:43
sschildhauer and others added 2 commits April 8, 2026 16:46
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Comment thread tests/testthat/test-print.sr_model.R
Copilot AI review requested due to automatic review settings April 8, 2026 10:17
@sschildhauer sschildhauer requested a review from d-morrison April 8, 2026 10:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Comment thread R/print.sr_model.R Outdated
Comment thread R/print.sr_model.R
Comment on lines +17 to +23
print.sr_model <- function(x,
print_tbl = FALSE,
...) { # nolint
if (print_tbl) {
x <- dplyr::as_tibble(x)
print(x)
invisible(x)
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment thread R/print.sr_model.R Outdated
Comment on lines +5 to +45
#' 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)
}
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
#' 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)
}

Copilot uses AI. Check for mistakes.
Comment thread NEWS.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 8, 2026 10:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread R/print.sr_model.R
Comment thread tests/testthat/test-print.sr_model.R
@sschildhauer
Copy link
Copy Markdown
Collaborator Author

@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.

Copilot AI and others added 2 commits April 8, 2026 11:04
Copilot AI review requested due to automatic review settings April 8, 2026 11:06
@sschildhauer sschildhauer review requested due to automatic review settings April 8, 2026 11:06
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 8, 2026

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 producin...

Fixed in commit 1a97278. The .data usage in tidyselect contexts (.by in summarise(), names_from and values_from in pivot_wider()) was replaced with string column names: c("Stratification", "Iso_type", "Parameter"), "Parameter", and "median_val" respectively. The .data$ usage in data-masking contexts (filter(), arrange(), and summary expressions) is unchanged as that usage is correct.

@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::docum...

Fixed in commit 1a97278. The Rd files were out of sync because devtools::document() hadn't been run after recent roxygen2 comment changes. Running it updated man/print.sr_model.Rd, man/run_mod.Rd, man/nepal_sees_jags_output.Rd, man/plot_predicted_curve.Rd, and man/use_att_names.Rd. Also fixed the [dplyr::tbl_df] reference in print.sr_model.R to use [tibble::tbl_df] instead (commit ec88ab1).

Copilot AI changed the title Creating default print methods for class sr_model Fix tidyselect deprecation, regenerate docs, add missing snapshot for print.sr_model Apr 8, 2026
Copy link
Copy Markdown
Collaborator

@Kwan-Jenny Kwan-Jenny left a comment

Choose a reason for hiding this comment

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

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 in suppressWarnings() — 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.

@sschildhauer sschildhauer changed the title Fix tidyselect deprecation, regenerate docs, add missing snapshot for print.sr_model Creating a default print for sr_model Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants