Add `quantiles` parameter to `graph.curve.params()` by Kwan-Jenny · Pull Request #434 · UCD-SERG/serocalculator

Kwan-Jenny · 2025-04-19T00:59:20Z

This patch extends our antibody‐decay plotting helper by introducing a new quantiles argument, which lets the caller specify an arbitrary subset of quantiles to render (e.g. just the 50% band). All existing behavior is preserved when quantiles is unset. In order to exercise the new API we also:

Updated the function signature to accept a quantiles = NULL vector
Added logic to default quantiles to 10%, 50% and 90% when show_quantiles = TRUE
Refactored the code that builds the ggplot layers so that only the requested quantile lines are drawn
Tweaked the test suite (test-graph.curve.params.R) to include a fourth case that passes quantiles = c("med") and verifies the resulting plot

All existing examples and tests have been left intact, and no existing behavior changes when quantiles is not supplied. This new option gives callers full control over which percentile bands to display.

codecov · 2025-04-19T01:22:46Z

Codecov Report

Attention: Patch coverage is 91.56627% with 7 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
R/graph.curve.params.R	91.56%	7 Missing ⚠️

Files with missing lines	Coverage Δ
R/graph.curve.params.R	`92.96% <91.56%> (-0.20%)`	⬇️

…rams

github-actions · 2025-04-21T05:26:42Z

📕 Preview documentation for this PR has been cleaned up.

Kwan-Jenny · 2025-04-21T06:25:49Z

I am not sure why the “Check NEWS.md Changelog” keeps failing, even though I have added the update to the NEWS.md file.

This PR adds an input argument to control which quantile curves are displayed. However, I realized it would also be helpful to include an additional argument to control the color of the chains. Currently, each chain is plotted with a different color, but I would like to make this behavior optional — so users can choose between showing different colors for each chain or using the same color (e.g., black) for all chains regardless of how many there are.

d-morrison · 2025-04-21T18:22:03Z

I am not sure why the “Check NEWS.md Changelog” keeps failing, even though I have added the update to the NEWS.md file.

@Kwan-Jenny It was an out of date Github Actions workflow, sorry; fixed now! See UCD-SERG/serodynamics#95 for details

d-morrison · 2025-04-21T18:24:46Z

I realized it would also be helpful to include an additional argument to control the color of the chains. Currently, each chain is plotted with a different color, but I would like to make this behavior optional — so users can choose between showing different colors for each chain or using the same color (e.g., black) for all chains regardless of how many there are.

@Kwan-Jenny I agree! Let's start a separate PR for this feature; I created Issue #436 to track it.

d-morrison · 2025-04-21T20:38:57Z

  show_quantiles = TRUE,
  show_all_curves = FALSE,
-  alpha_samples = 0.3
+  alpha_samples = 0.3,
+  quantiles = NULL


I'm not sure whether we need both the show_quantiles and quantiles arguments; I think we might want to just have quantiles, with default value c("p10", "med", "p90"), and if the user sets quantiles = NULL, that means don't show the quantiles.

@kaiemjoy @kristinawlai @sschildhauer thoughts?

also, maybe we should have the quantiles argument be numeric instead of character strings?

also, maybe we should have the quantiles argument be numeric instead of character strings?

... and then the user could specify any arbitrary set of quantiles, not just ones we hard-coded

@d-morrison I completely agree. I added an extra input argument to avoid altering the original code, as I didn’t want to risk breaking anything. But I see now that your approach is more user-friendly and will likely make things easier for others.

@Kwan-Jenny that's a good concern!

In this particular case, the show_quantiles argument is a relatively recent addition, added after the last CRAN release, so I think we can just replace it, as long as we explain the change in NEWS.md.

…iles

sschildhauer

Hi Kwan, after implementing the below suggestions please let me know if you want me to review again and I would be happy to jump back in.

sschildhauer

Not much left! Only one simple documentation change, then I think good to request from Ezra.

Co-authored-by: Samuel Schildhauer <165851188+sschildhauer@users.noreply.github.com>

…calculator into update_graph_curve erro fixed

d-morrison

Looking good, but I have some questions; please see comments.

d-morrison · 2025-06-24T01:27:10Z

-      min = min(.data$res, 0.9),
-      max = max(.data$res, 2000)
+      min = min(.data$res, na.rm = TRUE),
+      max = max(.data$res, na.rm = TRUE)


explain this change?

I’m not entirely sure since this part was coded a few months ago, but I think I changed it to make the function more general and data-driven. Instead of using hardcoded limits, it now reflects the actual range of the model output, and na.rm = TRUE helps handle any potential missing values. It seemed like a safer and more flexible approach for broader use.

d-morrison · 2025-06-24T01:32:33Z

-#' show(plot2)
+#' plot2 <- graph.curve.params(curve, show_all_curves = TRUE,
+#' quantiles = c(0.1, 0.5, 0.9))
+#' print(plot2)


please use the external example file approach (as Sam suggested, for any examples ≥3 lines long); that way, the examples will get linted properly.

d-morrison · 2025-06-24T01:34:57Z

-      )
-    )
-
+  # FIXED: avoid use of dot in slice() context


not sure what this comment means; is it still relevant?

comments in code files should explain what the code currently does; the comments should not discuss how the code previously looked. Comments like this one should be posted on GitHub in the PR.

I somehow changed it, possibly due to masking or linting issues at that time — I'm not entirely sure, so I reverted it to the original.

d-morrison · 2025-06-24T01:37:59Z

-  dT <- # nolint: object_linter
-    data.frame(t = tx2) |>
-    mutate(ID = dplyr::row_number()) |>
-    pivot_wider(
-      names_from = "ID",
-      values_from = "t",
-      names_prefix = "time"
-    ) |>
-    dplyr::slice(
-      rep(
-        seq_len(dplyr::n()),
-        each = nrow(d)
-      )
-    )
-
+  # FIXED: avoid use of dot in slice() context
+  dT_base <- data.frame(t = tx2) |>  # nolint: object_name_linter
+    dplyr::mutate(ID = dplyr::row_number()) |>
+    tidyr::pivot_wider(names_from = "ID",
+                       values_from = "t",
+                       names_prefix = "time")
+  dT <- dT_base |> # nolint: object_name_linter
+    dplyr::slice(rep(seq_len(nrow(dT_base)), each = nrow(d)))


I can't tell what changed here or why; please explain in a comment?

PS - it would help if the formatting changed as little as possible, so that it's easier to tell which parts are the same and which parts are changed.

I somehow changed it as above, possibly due to masking or linting issues at the time — I'm not entirely sure, so I reverted it back to the original here as well.

d-morrison · 2025-07-02T22:50:33Z

-    message(
-      "Graphing curves for antigen isotypes: ",
-      paste(antigen_isos, collapse = ", ")
-    )
+    message("Graphing curves for antigen isotypes: ",
+            paste(antigen_isos, collapse = ", "))


why did you reformat these lines? was there a lint? If not, please try to avoid changing lines that are unrelated to the goals of your PR.

I think I accidentally made edits while working with the linter, so I reverted the changes and restored the original formatting.

d-morrison

Please wrap up this PR asap; there are additional extensions we want to make to this function, and if we make those extensions first, it will probably cause merge conflicts for this PR.

d-morrison · 2025-07-02T22:52:21Z

-      )
-    )
-
+  # FIXED: avoid use of dot in slice() context


comments in code files should explain what the code currently does; the comments should not discuss how the code previously looked. Comments like this one should be posted on GitHub in the PR.

d-morrison · 2025-07-02T22:53:17Z

-  dT <- # nolint: object_linter
-    data.frame(t = tx2) |>
-    mutate(ID = dplyr::row_number()) |>
-    pivot_wider(
-      names_from = "ID",
-      values_from = "t",
-      names_prefix = "time"
-    ) |>
-    dplyr::slice(
-      rep(
-        seq_len(dplyr::n()),
-        each = nrow(d)
-      )
-    )
-
+  # FIXED: avoid use of dot in slice() context
+  dT_base <- data.frame(t = tx2) |>  # nolint: object_name_linter
+    dplyr::mutate(ID = dplyr::row_number()) |>
+    tidyr::pivot_wider(names_from = "ID",
+                       values_from = "t",
+                       names_prefix = "time")
+  dT <- dT_base |> # nolint: object_name_linter
+    dplyr::slice(rep(seq_len(nrow(dT_base)), each = nrow(d)))


PS - it would help if the formatting changed as little as possible, so that it's easier to tell which parts are the same and which parts are changed.

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

…dvisor feedback

…code, undid this

…t I undid it so I can keep original

…'m not entirely sure, so I reverted it to the original.

d-morrison

Thanks! Looks good. I handled the merge conflicts in #454; will merge both shortly!

Kwan-Jenny added 3 commits April 18, 2025 17:45

# add one more input quantiles

53491ce

# update test file of graph.curve.params

ed4b2ba

# version update

0ee71d5

Kwan-Jenny requested a review from d-morrison April 19, 2025 00:59

Kwan-Jenny added 2 commits April 18, 2025 18:13

update NEWS.md file

e12d269

# devtools::test() to generate test file of updating part in figure

55f0294

d-morrison requested a review from sschildhauer April 19, 2025 06:00

# Add updated testthat snapshot files after changes to graph.curve.pa…

6d6d8b1

…rams

Kwan-Jenny added 2 commits April 20, 2025 22:47

# Add updated snapshot .csv files for graph.curve.params changes

26902a4

# Fix lint issues in graph.curve.params.R

bb7d94e

Merge branch 'main' into update_graph_curve

d951038

d-morrison mentioned this pull request Apr 21, 2025

include an additional argument in graph.curve.params() to control the color of the chains. #436

Open

d-morrison reviewed Apr 21, 2025

View reviewed changes

Kwan-Jenny added 3 commits April 24, 2025 15:19

# Refactor graph.curve.params(): numeric quantiles, remove show_quant…

f14b4b9

…iles

# Accept change on svg files

225939a

# back to original code

3dd5078

d-morrison changed the title ~~Add quantiles parameter to graph.curve.params() and update unit tests~~ Add quantiles parameter to graph.curve.params() Apr 24, 2025

Kwan-Jenny added 8 commits April 24, 2025 16:45

# Back to original code for labelling MCMC chain part

26c72c2

# Qualify any column names inside

5f28a62

# tidyselect deprecation warnings

4b1da96

# update space in function to avoid error in lintr

f3ec434

# update error from lintr issues

a95f10c

# spaces

9dd4229

# spaces again

fe2cbdd

space again

e8945ee

Kwan-Jenny added 2 commits April 30, 2025 17:16

# update based on Sam's suggestion

1c08c7a

# forgot to do devtools::document()

752949c

Kwan-Jenny requested review from sschildhauer and removed request for d-morrison May 7, 2025 01:04

sschildhauer requested changes May 12, 2025

View reviewed changes

Kwan-Jenny added 2 commits May 17, 2025 08:12

# update code based on Sam's suggestions

92b2442

# forgot update documentation

e62c82c

Kwan-Jenny requested a review from d-morrison May 17, 2025 16:05

d-morrison requested review from sschildhauer and removed request for d-morrison May 23, 2025 01:36

sschildhauer reviewed May 29, 2025

View reviewed changes

Comment thread R/graph.curve.params.R Outdated

Kwan-Jenny and others added 3 commits May 30, 2025 14:10

Update R/graph.curve.params.R

f27e31c

Co-authored-by: Samuel Schildhauer <165851188+sschildhauer@users.noreply.github.com>

# Sam's review + devtools::document()

15ecabb

Merge branch 'update_graph_curve' of https://github.com/UCD-SERG/sero…

1339caf

…calculator into update_graph_curve erro fixed

Kwan-Jenny requested a review from d-morrison May 30, 2025 22:01

Merge branch 'main' into update_graph_curve

1abd307

d-morrison requested changes Jun 24, 2025

View reviewed changes

d-morrison reviewed Jul 2, 2025

View reviewed changes

d-morrison requested changes Jul 2, 2025

View reviewed changes

Kwan-Jenny and others added 7 commits July 3, 2025 14:28

Update NEWS.md

f72ead0

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

Update R/graph.curve.params.R

ab3fe13

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

# add external exmaple file in examples folder

d9bbaec

# Reverted to original formatting — avoided unnecessary changes per a…

74f412d

…dvisor feedback

# Didn't recognize ab1 function and create ab function separately in …

06abe06

…code, undid this

# I changed 'object' to 'curve_params' which term is more familiar bu…

db37250

…t I undid it so I can keep original

# I somehow changed it, possibly due to masking or linting issues — I…

4971cf6

…'m not entirely sure, so I reverted it to the original.

Kwan-Jenny requested a review from d-morrison July 4, 2025 23:02

d-morrison reviewed Jul 4, 2025

View reviewed changes

d-morrison merged commit c27b070 into main Jul 5, 2025
1 check passed

d-morrison deleted the update_graph_curve branch July 5, 2025 00:14

Uh oh!

Conversation

Kwan-Jenny commented Apr 19, 2025

Uh oh!

codecov Bot commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kwan-Jenny commented Apr 21, 2025

Uh oh!

d-morrison commented Apr 21, 2025

Uh oh!

d-morrison commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sschildhauer left a comment

Choose a reason for hiding this comment

Uh oh!

sschildhauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d-morrison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-morrison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-morrison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov Bot commented Apr 19, 2025 •

edited

Loading

github-actions Bot commented Apr 21, 2025 •

edited

Loading

d-morrison commented Apr 21, 2025 •

edited

Loading