DRYify Map And Forecast Functions #71

O957 · 2025-12-05T15:00:41Z

This PR:

Adds two new functions (in files) write_ref_date_summary.R and summarize_ref_date_forecasts.R, which DRYify processes across get_forecast_data.R and get_map_data.R.

O957 · 2025-12-05T15:03:08Z

lintr produced:

[object_length_linter] Variable and function names should not be longer than 30 characters.
write_ref_date_summary_ensemble <- function(
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Error: File R/write_ref_date_summary.R is not lint free
Execution halted

I am going to update the lintr behavior.

codecov-commenter · 2025-12-05T15:03:18Z

Codecov Report

❌ Patch coverage is 0.47847% with 208 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.74%. Comparing base (cfad627) to head (56aa356).

Files with missing lines	Patch %	Lines
R/write_ref_date_summary.R	0.00%	105 Missing ⚠️
R/summarize_ref_date_forecasts.R	0.00%	97 Missing ⚠️
R/check_authorized_users.R	0.00%	1 Missing ⚠️
R/check_changes_for_autoapproval.R	0.00%	1 Missing ⚠️
R/generate_hub_baselines.R	0.00%	1 Missing ⚠️
R/generate_hub_ensemble.R	0.00%	1 Missing ⚠️
R/generate_oracle_output.R	0.00%	1 Missing ⚠️
R/update_authorized_users.R	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main     #71      +/-   ##
========================================
+ Coverage   8.32%   8.74%   +0.42%     
========================================
  Files         10      10              
  Lines        769     743      -26     
========================================
+ Hits          64      65       +1     
+ Misses       705     678      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…a-and-get_forecast_data

R/write_ref_date_summary.R

dylanhmorris · 2025-12-05T15:14:10Z

R/summarize_ref_date_forecasts.R

+  checkmate::assert_character(excluded_locations)
+  checkmate::assert_character(targets, null.ok = TRUE)
+  checkmate::assert_character(model_ids, null.ok = TRUE)
+  checkmate::assert_data_frame(population_data, null.ok = TRUE)


I think these type checks can all be omitted. AFAICT, everything we do with them below is can rely on duck typing principles

dylanhmorris · 2025-12-05T15:17:53Z

R/summarize_ref_date_forecasts.R

+  forecasts_data <- forecasts_data |>
+    dplyr::arrange(.data$location_sort_order, .data$location_name)
+
+  if (include_metadata && !is.null(model_metadata)) {


Since you filter to desired columns when producing the output tables, you can always fetch and include the metadata, which will avoid extra switches

dylanhmorris · 2025-12-05T15:21:15Z

R/summarize_ref_date_forecasts.R

+  checkmate::assert_data_frame(population_data, null.ok = TRUE)
+  checkmate::assert_logical(include_metadata, len = 1)
+
+  if (!is.null(population_data)) {


Similar to https://github.com/CDCgov/hubhelpr/pull/71/files#r2593041996, let's just make the population data table required for this function (even if some downstream tables we may choose to produce don't end up including the per 100k columns(). We'll always have the population data available and that way the output data frame for this function has a predictable set of columns

dylanhmorris · 2025-12-05T15:22:21Z

R/summarize_ref_date_forecasts.R

+  if (!is.null(model_ids)) {
+    current_forecasts <- current_forecasts |>
+      dplyr::filter(.data$model_id %in% !!model_ids)
+  }


Move this down to alongside the target filter and use nullable_comparison

dylanhmorris · 2025-12-05T15:29:23Z

R/write_ref_date_summary.R

+#' model metadata (team_name, model_name). Default: TRUE.
+#' @param column_selection named character vector
+#' specifying which columns to select and rename. If NULL,
+#' includes all columns. Default: NULL.


Strictly doesn't have to be named; if it isn't you just wont get renamings. You can even mix and match a bit, though there are quirks. See last example

Compare

df <- tibble::tibble(x=1:5,y=10:14) dplyr::select(df, tidyselect::all_of(c(a="x", b="y", c="x"))) dplyr::select(df, tidyselect::all_of(c("x", "y", "x"))) dplyr::select(df, tidyselect::all_of(c("y", "x", c="x")))

dylanhmorris · 2025-12-05T15:33:20Z

R/write_ref_date_summary.R

+
+  if (!is.null(column_selection)) {
+    summary_data <- summary_data |>
+      dplyr::select(!!!column_selection)


Suggested change

dplyr::select(!!!column_selection)

dplyr::select(tidyselect::all_of(column_selection))

dylanhmorris · 2025-12-05T15:34:02Z

R/write_ref_date_summary.R

+  model_ids = NULL,
+  population_data = NULL,
+  include_metadata = TRUE,
+  column_selection = NULL


If you have this default to tidyselect::everything() you can avoid the if is NULL check

Suggested change

column_selection = NULL

column_selection = tidyselect::everything()

dylanhmorris · 2025-12-05T15:34:45Z

R/write_ref_date_summary.R

+  checkmate::assert_string(file_suffix)
+  checkmate::assert_character(column_selection, null.ok = TRUE)


As above

Suggested change

checkmate::assert_string(file_suffix)

checkmate::assert_character(column_selection, null.ok = TRUE)

dylanhmorris · 2025-12-05T15:36:37Z

R/write_ref_date_summary.R

+  checkmate::assert_data_frame(population_data)
+  checkmate::assert_names(
+    colnames(population_data),
+    must.include = c("location", "population")
+  )


Defer checking the population data to summarize_ref_date_forecasts

Suggested change

checkmate::assert_data_frame(population_data)

checkmate::assert_names(

colnames(population_data),

must.include = c("location", "population")

)

dylanhmorris · 2025-12-05T15:39:23Z

R/get_forecast_data.R

Is the thought to retain this for backward compatibility? I think it is not used enough yet to make this worthwhile. I would just delete and make sure we port over the (currently one) hub that uses it.

I originally wrote a deprecation comment on both get_*.R files, but I left them with minimal comment, in hopes that you might afford some remarks. I think deleting them makes the most sense (given there is not really a user base who've become accustomed to them) but didn't want to make this decision myself.

dylanhmorris · 2025-12-05T15:39:38Z

R/get_map_data.R

Same comment as get_forecast_data.R

dylanhmorris

This is looking good. Thanks, @O957. A few changes needed.

dylanhmorris · 2025-12-05T21:33:24Z

R/summarize_ref_date_forecasts.R

@@ -0,0 +1,141 @@
+#' Summarize forecast hub data for a specific reference
+#' date. This function generates a tibble of forecast data


The first line becomes the header in R function descriptions, so it is worth having a line break.

Suggested change

#' date. This function generates a tibble of forecast data

#' date.

#'

#' This function generates a tibble of forecast data

dylanhmorris · 2025-12-05T21:34:04Z

R/summarize_ref_date_forecasts.R

+  excluded_locations = character(0),
+  targets = NULL,
+  model_ids = NULL,
+  population_data


Put this before the keyword args

dylanhmorris · 2025-12-05T21:34:37Z

R/summarize_ref_date_forecasts.R

+
+  model_metadata <- hubData::load_model_metadata(
+    base_hub_path,
+    model_ids = NULL


Why not use the user-passed model_ids here?

dylanhmorris · 2025-12-05T21:36:16Z

R/summarize_ref_date_forecasts.R

+      forecast_due_date_formatted = format(.data$forecast_due_date, "%B %d, %Y")
+    )
+
+  forecasts_data


I prefer explicit returns. Some R linters disagree on the default setting, but this can be adjusted.

Suggested change

forecasts_data

return(forecasts_data)

I added explicit returns elsewhere across the codebase where they didn't occur.

dylanhmorris · 2025-12-05T21:39:38Z

R/write_ref_date_summary.R

+#' @param column_selection character vector specifying
+#' which columns to select. Uses tidyselect semantics.
+#' Default: tidyselect::everything().


Suggested change

#' @param column_selection character vector specifying

#' which columns to select. Uses tidyselect semantics.

#' Default: tidyselect::everything().

#' @param column_selection Columns to include in the output table.

#' Uses [tidy selection](https://dplyr.tidyverse.org/articles/programming.html).

#' Default: [tidyselect::everything()].

R/write_ref_date_summary.R

dylanhmorris

This is looking good. A few remaining questions.

…dd line breaks

O957 · 2025-12-08T17:49:57Z

This is looking good. A few remaining questions.

I think I got everything, including the renamings. Thank you thus far for the review comments. There were also a few more lines breaks that I failed to add for other functions; I forgot about the fact that these first lines become the R function descriptions.

…a-and-get_forecast_data

R/generate_oracle_output.R

R/summarize_ref_date_forecasts.R

R/write_ref_date_summary.R

dylanhmorris

Thanks, @O957!

dylanhmorris · 2025-12-09T18:56:21Z

@O957 I think you can merge. The CI failure looks to be a runner issue, not an issue with the codebase

O957 · 2025-12-09T19:00:21Z

@O957 I think you can merge. The CI failure looks to be a runner issue, not an issue with the codebase

Thank you, I thought so, but got distracted after I re-ran the failed job.

attempt to dryify code

84f35e6

O957 self-assigned this Dec 5, 2025

O957 requested review from dylanhmorris and sbidari as code owners December 5, 2025 15:00

O957 linked an issue Dec 5, 2025 that may be closed by this pull request

DRYify get_map_data() and get_forecast_data(). #70

Closed

Merge remote-tracking branch 'origin/main' into 70-dryify-get_map_dat…

1c7f930

…a-and-get_forecast_data

dylanhmorris reviewed Dec 5, 2025

View reviewed changes

R/write_ref_date_summary.R Outdated Show resolved Hide resolved

dylanhmorris reviewed Dec 5, 2025

View reviewed changes

update function description

98b3366

dylanhmorris reviewed Dec 5, 2025

View reviewed changes

R/get_map_data.R Outdated

Copy link

Contributor

dylanhmorris Dec 5, 2025

Choose a reason for hiding this comment

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

Same comment as get_forecast_data.R

dylanhmorris requested changes Dec 5, 2025

View reviewed changes

attempt to try review comments

dd1eb1a

O957 requested a review from dylanhmorris December 5, 2025 19:11

dylanhmorris reviewed Dec 5, 2025

View reviewed changes

R/write_ref_date_summary.R Show resolved Hide resolved

dylanhmorris requested changes Dec 5, 2025

View reviewed changes

O957 added 2 commits December 8, 2025 12:20

make return statements explicit; change param order; update params; a…

cd7f618

…dd line breaks

run devtools document

8eb52f7

O957 requested a review from dylanhmorris December 8, 2025 17:48

O957 added 2 commits December 8, 2025 13:12

Merge remote-tracking branch 'origin/main' into 70-dryify-get_map_dat…

a44339f

…a-and-get_forecast_data

Merge remote-tracking branch 'origin/main' into 70-dryify-get_map_dat…

ec67674

…a-and-get_forecast_data

dylanhmorris mentioned this pull request Dec 9, 2025

Add Function For Generating Forecast Webpage Text #38

Open

7 tasks

dylanhmorris reviewed Dec 9, 2025

View reviewed changes

R/generate_oracle_output.R Show resolved Hide resolved

dylanhmorris reviewed Dec 9, 2025

View reviewed changes

R/summarize_ref_date_forecasts.R Show resolved Hide resolved

dylanhmorris reviewed Dec 9, 2025

View reviewed changes

R/write_ref_date_summary.R Outdated Show resolved Hide resolved

dylanhmorris reviewed Dec 9, 2025

View reviewed changes

R/write_ref_date_summary.R Outdated Show resolved Hide resolved

dylanhmorris approved these changes Dec 9, 2025

View reviewed changes

O957 added 3 commits December 9, 2025 13:28

add dhm suggested edits

6391947

typos edit

a7cb9cb

run devtools document

56aa356

O957 merged commit 02fe0c9 into main Dec 9, 2025
8 of 10 checks passed

O957 deleted the 70-dryify-get_map_data-and-get_forecast_data branch December 9, 2025 19:00

	dplyr::select(!!!column_selection)
	dplyr::select(tidyselect::all_of(column_selection))

	column_selection = NULL
	column_selection = tidyselect::everything()

		checkmate::assert_string(file_suffix)
		checkmate::assert_character(column_selection, null.ok = TRUE)

		@@ -0,0 +1,141 @@
		#' Summarize forecast hub data for a specific reference
		#' date. This function generates a tibble of forecast data

DRYify Map And Forecast Functions #71

DRYify Map And Forecast Functions #71

Uh oh!

Conversation

O957 commented Dec 5, 2025

Uh oh!

O957 commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

dylanhmorris Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanhmorris Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanhmorris 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!

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!

Uh oh!

dylanhmorris left a comment

Choose a reason for hiding this comment

Uh oh!

O957 commented Dec 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylanhmorris left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanhmorris commented Dec 9, 2025

Uh oh!

O957 commented Dec 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

O957 commented Dec 5, 2025 •

edited

Loading

codecov-commenter commented Dec 5, 2025 •

edited

Loading

dylanhmorris Dec 5, 2025 •

edited

Loading

dylanhmorris Dec 5, 2025 •

edited

Loading

dylanhmorris left a comment •

edited

Loading