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

Make interval_range in interval_score more robust #277

Conversation

adrian-lison
Copy link

This PR changes the interval_range argument from percentage scale to unit scale with the intention of making the function interval_score more robust.

Pros:

  • Typical wrong inputs (outside of the range [0,1[ ) can now be detected. Before, if a user accidentally specified 0.5 (meaning 50%), the function would just compute the score for the 0.5% range. Now, if a user accidentally specifies 50 (meaning 50%), there will be an error.
  • More consistency with other arguments / variables, e.g. alpha or the quantiles argument in quantile_score are also on the unit scale

Cons:

  • Would introduce a breaking change to the interface. However, because of the new validity check, there would likely be no silent errors (users that were specifying the range on the percentage scale will receive an error).

@nikosbosse
Copy link
Contributor

Hi Adrian, thanks a lot for the PR!
I think in principle it's a good idea. Some more thoughts:

If we want to change to the unit scale

  • we'd need to make the change in all score() as well
  • we could think about adding a warning to the function for a while (hmm although I guess users would get an error anyway in most instances...)

If we wanted to stick to the current function

  • we could throw a warning if a user wants to provides something like range = 0.5 (who in their right mind would ever want to score a forecast for a 0.4975 and a 0.5025 prediction interval?). Could also be one of those warnings that come up once every 8h.

If we just want to save users from making mistakes, then maybe option 2 and just adding a warning is easier. But agree that for consistency your option would be better. What do you think?

@nikosbosse
Copy link
Contributor

Just made a PR for option 2 (as it's just a lot easier to implement for now and we could still make the change you suggested later) in #281

@seabbs
Copy link
Contributor

seabbs commented May 10, 2023

Hallo everyone, what is going on here? Am I right in thinking Nikos you went a different way to solve this? Should we be closing this PR?

@nikosbosse
Copy link
Contributor

Yes I think we should be closing it for now. @adrian-lison if you feel strongly about it I think we should create an issue and a new PR / reopen this PR at a later point in time.

@nikosbosse nikosbosse closed this May 10, 2023
@seabbs
Copy link
Contributor

seabbs commented May 10, 2023

Thanks again for this @adrian-lison!

@nikosbosse
Copy link
Contributor

Yes, thank you @adrian-lison! I think this was really helpful. Still happy to continue thinking more about the full version of what you suggested!

@adrian-lison
Copy link
Author

No worries @nikosbosse, and I think your PR already addresses the main concern!

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.

None yet

3 participants