-
Notifications
You must be signed in to change notification settings - Fork 3
Issue 788: Add S3 dispatch for model sample processing #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #790 +/- ##
==========================================
+ Coverage 39.43% 41.71% +2.27%
==========================================
Files 30 30
Lines 2713 2817 +104
==========================================
+ Hits 1070 1175 +105
+ Misses 1643 1642 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Updates `process_loc_forecast` to delegate to the new interface when `model_name` is provided, and adds comprehensive tests for model type detection, S3 dispatch, and interface validation.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
6540e40 to
1389819
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces an extensible S3 dispatch system for processing location forecasts, enabling support for multiple model types and simplifying future model additions. The implementation adds a new model_name parameter to process_loc_forecast that leverages S3 method dispatch via process_model_samples, while maintaining backward compatibility with the existing interface.
Key changes:
- Added S3 generic
process_model_sampleswith methods for "pyrenew" and "timeseries" model types - Implemented
detect_model_typehelper to auto-detect model type from naming conventions - Created new
process_forecastfunction as simplified interface using the S3 dispatch system
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hewr/R/process_loc_forecast.R | Core implementation of S3 dispatch system, including process_model_samples generic/methods, detect_model_type helper, and new process_forecast function with updated process_loc_forecast routing logic |
| hewr/tests/testthat/test_process_loc_forecast.R | Tests for S3 dispatch functionality, model type detection, and validation of both new and legacy interfaces |
| hewr/tests/testthat/test_timeseries_utils.R | Comprehensive test suite for timeseries utility functions including formatting, aggregation, and proportion calculations |
| hewr/man/*.Rd | Documentation files for new exported functions and S3 methods |
| hewr/NAMESPACE | Updated exports to include new S3 methods and functions |
| hewr/DESCRIPTION | RoxygenNote version bump from 7.3.2 to 7.3.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is probably room to make some better abstractions, but I'm in favor of unblocking #783, so I have approved this PR and will work on some parallel changes in another PR.
This reverts commit f69b358.
|
This broke the pipeline. Missed it due to insufficient testing (#791) |
* Add S3 dispatch for model sample processing and new interface Updates `process_loc_forecast` to delegate to the new interface when `model_name` is provided, and adds comprehensive tests for model type detection, S3 dispatch, and interface validation. * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add S3 method export and refresh roxygen * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix linting * applied air formatting * shorten line for reasons * Avoid method warning * up coverage to pass CI * add method smoke tests * refactor tests * trigger ci * Revert "trigger ci" This reverts commit 8b5cfd4. * Update plot_and_save_loc_forecast.R --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
This PR adds an extensible interface for processing location forecasts to support multiple model types and make easier to do future expansion, which unblocks #783 .
Details
I've added a new
model_nameargument toprocess_loc_forecastfunction, if this is notNAthen it lowers toprocess_forecastfunction and if it isNA(as default) then current logic is untouched.process_forecastfunction can either be supplied amodel_typeto dispatch on aprocess_model_samplesmethod or it detects from model name with thedetect_model_typehelper func. The functions then gathers the same data asprocess_loc_forecastand then dispatches the appropriate processing on the raw model output.process_model_samplesmethods for"pyrenew"and"timeseries"model types, so this should work with egmodel_name = "pyrenew_e"but I've also kept legacy interface for backwards compat.