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

Add separate functions for wis components #397

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

nikosbosse
Copy link
Contributor

This PR

  • adds 3 new functions, dispersion(), overprediction() and underprediction() to compute WIS components.
  • updates the documentation for wis()

At the moment the computations are a bit wasteful as all of the component functions just call wis() and only use the relevant info from that. We could redesign that in the future, but for now I didn't think it was worth it given that the function is otherwise not very computation-heavy.

Also look how modular the PR is!

@nikosbosse nikosbosse requested a review from seabbs November 9, 2023 14:46
@nikosbosse nikosbosse changed the base branch from main to update-quantile-metrics November 9, 2023 14:47
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

In the review note you should be linking to an issue that explains why you are doing this for the user!

This PR has several linting issues that need resolving.

Do you want to expose wis arguments in these functions?

These should have trivial tests now as there is no reason not to do so.

#' `dispersion()`: a numeric vector with dispersion values (one per observation)
#' @export
#' @rdname wis
dispersion <- function(observed, predicted, quantile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do these intentionally not expose any of the arguments of WIS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh no. These are due to me being stupid. Changed the code.

inst/create-list-available-forecasts.R Show resolved Hide resolved
Base automatically changed from update-quantile-metrics to rework-add_coverage() November 15, 2023 14:37
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM

@nikosbosse nikosbosse merged commit 6cb1c3e into rework-add_coverage() Nov 15, 2023
@nikosbosse nikosbosse deleted the add-wis-components branch November 15, 2023 17:07
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.

2 participants