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

Handling case in WIS where quantile levels don't form valid intervals #943

Merged
merged 5 commits into from
Oct 8, 2024

Conversation

nikosbosse
Copy link
Contributor

Description

This PR closes #926.

As discussed in #926, this PR lets wis() error by default if not all quantile levels form valid prediction intervals. I realised that there was a na.rm argument already (it was just set to TRUE). I think FALSE is a better default.
If the quantile levels aren't symmetric, the function errors immediately.

(Note that there is a second way in which na.rm can be relevant: if a specific quantile is missing for a forecast. With na.rm = TRUE NA values will be dropped and otherwise the output is NA.)

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.

@seabbs
Copy link
Contributor

seabbs commented Oct 8, 2024

Kätzchen spielt im Hof
Miau, miau, ruft es leise
Mutters Strickzeug tanzt

@seabbs
Copy link
Contributor

seabbs commented Oct 8, 2024

Note that there is a second way in which na.rm can be relevant: if a specific quantile is missing for a forecast. With na.rm = TRUE NA values will be dropped and otherwise the output is NA.)

This was the motivation for the previous setting wasn't it because historically hubs allowed a mixed set of quantiles to be submitted and we wanted to support that?

@seabbs
Copy link
Contributor

seabbs commented Oct 8, 2024

Alte Katze schläft
Erinnerung an Spiele
Strickzeug längst verblasst

NEWS.md Outdated Show resolved Hide resolved
R/metrics-quantile.R Outdated Show resolved Hide resolved
@nikosbosse
Copy link
Contributor Author

Kätzchen spielt im Hof
Miau, miau, ruft es leise
Mutters Strickzeug tanzt

I couldn't agree more.

This was the motivation for the previous setting wasn't it because historically hubs allowed a mixed set of quantiles to be submitted and we wanted to support that?

This is really more missing forecast values. If you had mixed sets of quantiles, then you'd get a warning in score(), but otherwise it should be fine.

@elray1 @nickreich are there any issues you'd expect from this PR?
TLDR: wis() errors if you supply quantile_levels like c(0.1, 0.4, 0.7) that don't form intervals. And if you have quantile_levels = c(0.1, 0.2, 0.8, 0.9) but predicted = c(2, 4, 8, NA) then it will result in NA by default.

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.

Nice looks good. A few linting issues flagged and maybe more verbosity in the news. Also, I have found a great use of AI. See if you can guess what the prompt was.

@seabbs
Copy link
Contributor

seabbs commented Oct 8, 2024

This is really more missing forecast values. If you had mixed sets of quantiles, then you'd get a warning in score(), but otherwise it should be fine.

Okay makes sense.

@nikosbosse
Copy link
Contributor Author

Also, I have found a great use of AI. See if you can guess what the prompt was.

1950's German folk poetry? :D

@seabbs
Copy link
Contributor

seabbs commented Oct 8, 2024

haiku from a 19th century german 9 year old and a haiku from the same 9 year old when they are 95

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.

Nice. LGTM

@seabbs seabbs merged commit dd609af into main Oct 8, 2024
9 checks passed
@seabbs seabbs deleted the asymmetric-intervals branch October 8, 2024 17:51
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.

Handling asymmetric intervals
2 participants