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

Testing: Make sure pit() and plot_pit() are correct #359

Closed
Tracked by #493
nikosbosse opened this issue Oct 26, 2023 · 8 comments · Fixed by #916
Closed
Tracked by #493

Testing: Make sure pit() and plot_pit() are correct #359

nikosbosse opened this issue Oct 26, 2023 · 8 comments · Fixed by #916
Assignees

Comments

@nikosbosse
Copy link
Contributor

Check the functions thoroughly and write more tests for them to make sure they are correct.

  • in the past, we sometimes had instances where the bins did not sum to 1
  • think about how pit_sample() fits in with the other metrics - it's return type is different, because it is a many-to-many relationship.

Maybe related: tidyverse/ggplot2#2323

@nikosbosse nikosbosse converted this from a draft issue Oct 26, 2023
@nikosbosse nikosbosse added this to the scoringutils 1.3 milestone Oct 26, 2023
@nikosbosse nikosbosse changed the title Make sure pit() and plot_pit() are correct Testing: Make sure pit() and plot_pit() are correct Nov 15, 2023
@nikosbosse
Copy link
Contributor Author

@sbfnk could we interest you in having a look at this one? :)

@sbfnk
Copy link
Contributor

sbfnk commented Apr 16, 2024

I'd be thrilled.

@sbfnk sbfnk self-assigned this Apr 16, 2024
@nikosbosse
Copy link
Contributor Author

In addition, found this in my notes:

  • get_pit should call pit_sample() in the sample case and quantile_coverage in the quantile case and we should be able to say that in the docs. Also probably get_pit() should not necessarily have the n_replicates arg.
  • plot_wis has this:
    #' @param relative_contributions Logical. Show relative contributions instead 
    #' of absolute contributions? Default is `FALSE` and this functionality is not 
    #' available yet.
    

But maybe that's a different issue...

@nikosbosse
Copy link
Contributor Author

@sbfnk I imagine the thrill may have died down since April where you so kindly and voluntarily volunteered to have a look at this :) Is this still something you'd be willing to have a look at?

@nikosbosse
Copy link
Contributor Author

I found an issue with the pit plot breaks:

 observed <- rnorm(30, mean = 1:30)
  predicted <- replicate(200, rnorm(n = 30, mean = 1:30))
  p2 <- plot_pit(pit, num_bins = 5)

produces this plot with its lovely 4 bins...
image

@nikosbosse nikosbosse mentioned this issue Sep 11, 2024
9 tasks
@sbfnk
Copy link
Contributor

sbfnk commented Sep 13, 2024

get_pit should call pit_sample() in the sample case and quantile_coverage in the quantile case and we should be able to say that in the docs. Also probably get_pit() should not necessarily have the n_replicates arg.

Wouldn't the easiest way to implement different behaviours by forecast type to make get_pit() an S3 method?

@nikosbosse
Copy link
Contributor Author

Wouldn't the easiest way to implement different behaviours by forecast type to make get_pit() an S3 method?

ahh. yeah... now that you say it that seems very obviously a good idea

@seabbs
Copy link
Contributor

seabbs commented Sep 16, 2024

when in doubt make it an s3 methods the new scoringutils catch phrase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants