Add basic plot_pred_obs_pointinterval() functionality#146
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 64.29% 66.92% +2.63%
==========================================
Files 28 28
Lines 1574 1657 +83
==========================================
+ Hits 1012 1109 +97
+ Misses 562 548 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is my preferred option. If
|
Went with this in f8d53ad. Decided it was worth avoiding making the user specify the horizon timescale |
Co-authored-by: Damon Bayer <xum8@cdc.gov>
damonbayer
left a comment
There was a problem hiding this comment.
Looks nice. Just a few small changes requested.
Co-authored-by: Damon Bayer <xum8@cdc.gov>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Ready for re-review @damonbayer |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a basic implementation of plot_pred_obs_pointintervals() to visualize median predictions and prediction intervals against observed data. Key changes include implementation of the new plotting function in R/plot_scorable_table.R, accompanying documentation in man/plot_pred_obs_pointintervals.Rd, updated tests in tests/testthat, and exporting the function via NAMESPACE.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/testthat/test_plot_predicted_observed.R | Added tests checking behavior when both horizon and reference_date are provided and the error when horizon is missing. |
| man/plot_pred_obs_pointintervals.Rd | New documentation for the introduced plotting function. |
| R/plot_scorable_table.R | New function implementation with handling for redundant forecast_date columns, facetting logic, and renaming. |
| NAMESPACE | Export of the new plotting function. |
Comments suppressed due to low confidence (2)
tests/testthat/test_plot_predicted_observed.R:102
- [nitpick] Comparing entire ggplot objects using expect_equal may be brittle. Consider comparing key components (such as the underlying data or layer mappings) to ensure the test focuses on relevant changes.
expect_equal(result, result_manual)
R/plot_scorable_table.R:370
- [nitpick] The variable name 'qi' is a bit ambiguous. Consider renaming it to a more descriptive name like 'quantile_intervals'.
qi <- quantile_table_to_median_qi(
Closes #97
EDIT: expand to see previous request for feedback, now addressed
Would like feedback on how automagically/not to handle the question of horizons and forecast dates (aka reference dates). At present, the plot is faceted. Facet row groups are given the by horizon data column. Facet column groups are given by the interaction of every data column that isn't one of the following: target end date, predicted, observed, quantile level, or horizon. This means that if any columns are redundant with horizon given target end date (e.g. a forecast date = target end date - horizon), each facet will contain at most one prediction and observation.
See the following reprex that demonstrates the current behavior:
Created on 2025-05-22 with reprex v2.1.1
We could
forecast_date/reference_datecolumn and do something with it, e.g.horizoncolumn and aforecast_datecolumn, error if both are present, compute horizon from forecast date + target date if neededforecast_datewhenhorizonis also present, use it as a fallback when horizon is absent