Skip to content
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

Issue #498 Implement a print function for forecast objects #584

Closed

Conversation

toshiakiasakura
Copy link
Collaborator

Description

This PR closes #498 .

I implemented a print function using S3 method to print out the inforamtion about forecast objects.
Currently support for forecast_binary, forecast_quantile, forecast_point, forecast_sampel , forecast_integer.

dat <- example_quantile %>%
  set_forecast_unit(c("location", "target_end_date", "target_type", "horizon", "model")) %>%
  as_forecast() 
print(dat)
#> ======================================== 
#>        Forecast data information         
#> ======================================== 
#> Forecast type:                  quantile 
#>
#> Protected columns:
#>      observed, quantile, predicted
#>
#> Forecast unit:
#>      location, target_end_date,
#>      target_type, horizon, model
#> 
#> ======================================== 
#>             data.table print             
#> ======================================== 
#>       observed quantile predicted location target_end_date target_type horizon
#>    1:   127300       NA        NA       DE      2021-01-02       Cases      NA
#>    2:     4534       NA        NA       DE      2021-01-02      Deaths      NA
#>    3:   154922       NA        NA       DE      2021-01-09       Cases      NA
#>    4:     6117       NA        NA       DE      2021-01-09      Deaths      NA
#>    5:   110183       NA        NA       DE      2021-01-16       Cases      NA
#>   ---                                                                         
#>20541:       78    0.850       352       IT      2021-07-24      Deaths       2
#>20542:       78    0.900       397       IT      2021-07-24      Deaths       2
#>20543:       78    0.950       499       IT      2021-07-24      Deaths       2
#>20544:       78    0.975       611       IT      2021-07-24      Deaths       2
#>20545:       78    0.990       719       IT      2021-07-24      Deaths       2

Potential further update is to include validated information, i.e. results from validate_general or including the number of NA (#581).

Due to the issue of #559, after passing through add_coverage, it does not correctly work, but instead if use the internal private function print_forecast_info, results look like

dat2 <- example_quantile %>%
  set_forecast_unit(c("location", "target_end_date", "target_type", "horizon", "model")) %>%
  as_forecast() %>% 
  add_coverage()
print_forecast_info(dat2)
#> ======================================== 
#>        Forecast data information         
#> ======================================== 
#> Forecast type:                  quantile 
#>
#> Protected columns:
#>      observed, quantile, predicted,
#>      range
#>
#> Metric columns (protected):
#>      interval_coverage,
#>      interval_coverage_deviation,
#>      quantile_coverage,
#>      quantile_coverage_deviation
#>
#> Forecast unit:
#>      location, target_end_date,
#>      target_type, horizon, model
#> 
#> ======================================== 
#>             data.table print             
#> ======================================== 
#> ~~~~~~~~

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

P.S. I am not sure it is right to send a pull request to main branch but I can not find develop branch...

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (2ff165f) 83.71% compared to head (448aa97) 83.25%.
Report is 93 commits behind head on main.

Files Patch % Lines
R/utils.R 0.00% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
- Coverage   83.71%   83.25%   -0.46%     
==========================================
  Files          21       21              
  Lines        1719     1750      +31     
==========================================
+ Hits         1439     1457      +18     
- Misses        280      293      +13     

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

@nikosbosse
Copy link
Contributor

nikosbosse commented Jan 8, 2024

Thank you so much for this PR, @toshiakiasakura! The actual functionality is really there and looks good. I have some additional thoughts that I think would make the code easier to maintain in the future.

My main piece of feedback is that I would suggest we simplify the current proposal a bit. The current code is nice but fairly complex: it introduces a new package dependency (stringr) and does some fairly complicated manipulations with the strings. The output does look nice, but complicated code makes things harder to maintain in the long run (for example, if we wanted to change something at some point, or if some function from stringr changed).

It might really be enough to do something like the following:

print.forecast_quantile <- function(x, ...) {
  forecast_type <- get_forecast_type(x)
  cat("Forecast type:\n")
  print(forecast_type)

  forecast_unit <- get_forecast_unit(x)
  cat("\nForecast unit:\n")
  print(forecast_unit)

  protected_columns <- get_protected_columns(x)
  cat("\nProtected columns:\n")
  print(protected_columns)

  metric_names <- get_metrics(x)
  if (!is.null(metric_names)) {
    cat("\nScoring rules used:\n")
    print(metric_names)
  }

  cat("\n")
  print(as.data.table(x, ...))
  return(invisible(x))
}

This would allow us to avoid the package dependency, defining another helper function, using lapply(), etc. The output may be slightly less visually appealing, but the code is a lot simpler.

By convention, the method should have the same arguments as the generic function, so x, and ... in this case (as it is print(x, ...). This convention for S3 classes is described in more detail here.

What do you think?

I've opened up 2 related issues / pull requests:

Merging in those two PRs will make this one easier because then we can simply call get_metrics() (or its new equivalent) inside this print method and also don't have to worry about add_coverage() anymore.
Update: Thanks to @seabbs those PRs have already been reviewed and merged in

@seabbs
Copy link
Contributor

seabbs commented Jan 8, 2024

This does look really nice! Just to weigh in that if moving package messaging to {cli} we might want to use it here as this is the kind of thing its really designed for.

@toshiakiasakura
Copy link
Collaborator Author

Thanks for both comments and updates for the branch.
So I will clean up and re-start with a very simple version of print function, following up the updates in the pretty printing version. For that purpuse I close this PR.

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.

Create a print method for forecast_binary, forecast_quantile etc.
3 participants