913: Functions for PIT histograms #949

sbfnk · 2024-10-16T11:08:34Z

Description

This PR closes #913.

It adds ~~plot_histogram_sample()~~ pit_histogram_sample() and get_pit_histogram() functions and implements the nonrandomised PIT for count data suggested by Czado et al.

It also removes get_pit() and pit_sample() - I don't think calculating PIT values themselves is very interesting or something anyone would ever want to do but if people disagree we could keep them in.

It also removes plot_pit(). Now that densities of the histogram can be obtained directly I would think that a user will be able to make their own plot.

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.

R/class-forecast-quantile.R

nikosbosse · 2024-10-20T13:45:24Z

Nice, thank you!
I suggested some cosmetic changes in #950.

Broader thoughts/questions:

I see the point you raised in get_ function names #948 - prefixing the functions with get_ indeed feels a bit weird.
- one alternative I could think of is changing names to pit_histogram.forecast_sample() and pit_histogram_sample()?
I'm wondering whether it makes sense to keep pit_histogram_sample() exported now that it returns a histogram rather than the raw pit values. But I think it does?
plot_histogram_sample() does not exist anymore, does it? The PR description mention still mentions it. I personally like having a plot function. At least I think it would be nice to provide the code as an example

sbfnk · 2024-10-21T08:22:58Z

one alternative I could think of is changing names to pit_histogram.forecast_sample() and pit_histogram_sample()?

This would be my suggestion (i.e. just remove the get_ bits) but probably best left for that issue to be addressed alongside any other function(s) that should get the same treatment.

I'm wondering whether it makes sense to keep pit_histogram_sample() exported now that it returns a histogram rather than the raw pit values. But I think it does?

Whatever we do I think it deserves the same treatment as related functions (e.g. bias_sample()) - assuming they would have some utility for those who don't want to go down the data.frame/S3 path perhaps keep them exported?

plot_histogram_sample() does not exist anymore, does it? The PR description mention still mentions it. I personally like having a plot function. At least I think it would be nice to provide the code as an example

It never existed. I meant pit_histogram_sample() and have now edited the PR description. My impression was with the new function structure plotting is now so trivial that it doesn't need its own function (and people can choose their favourite base R / ggplot / etc routine). There is an example in the manuscript:

example_sample_continuous |>
  as_forecast_sample() |>
  get_pit_histogram(by = c("model", "target_type")) |>
  ggplot(aes(x = mid, y = density)) +
  geom_col() +
  facet_grid(target_type ~ model) +
  labs(x = "Quantile", "Density")

Do you think this should be replicated somewhere else?

Co-authored-by: Sebastian Funk <[email protected]>

nikosbosse · 2024-10-21T10:44:34Z

Do you think this should be replicated somewhere else?

I'm leaning slightly towards a function (I would personally have to think more than 30 seconds about how to plot this + in order to plot this you need to know what a pit histogram needs to look like).

As an alternative I would suggest adding the code snippet to the function examples + adding a comment such as "For an example of how to plot the output, see the examples section" in the function docs.
I don't feel that strongly because we can always go from option 2 to option 1 in the future.

sbfnk · 2024-10-21T12:59:49Z

I'm leaning slightly towards a function (I would personally have to think more than 30 seconds about how to plot this + in order to plot this you need to know what a pit histogram needs to look like).

Thinking about this I think this is a bit more complicated in the new set up because the plot function doesn't necessarily know what to plot / facet (i.e. what was passed to by).

Alternatively we could change get_pit_histogram() to return objects of class histogram (i.e. same as returned by hist)? That would be nice as mids, bins etc. would all be taken care of, and there would be a simple plot() function. However, the interaction with by would be more complicated.

nikosbosse

Nice!

R/get-pit-histogram.R

nikosbosse · 2024-10-22T07:12:25Z

Shall we merge it?

seabbs · 2024-10-22T10:30:05Z

know what to plot / facet (i.e. what was passed to by).

In epinowcast the facetting is left as an exercise for the user which I like.

seabbs · 2024-10-22T10:31:48Z

NEWS.md

@@ -84,6 +84,7 @@ of our [original](https://doi.org/10.48550/arXiv.2205.07090) `scoringutils` pape
 - Removed `interval_coverage_sample()` as users are now expected to convert to a quantile format first before scoring.
 - Function `set_forecast_unit()` was deleted. Instead there is now a `forecast_unit` argument in `as_forecast_<type>()` as well as in `get_duplicate_forecasts()`.
 - Removed `interval_coverage_dev_quantile()`. Users can still access the difference between nominal and actual interval coverage using `get_coverage()`.
+- `pit()`, `pit_sample()` and `plot_pit()` have been removed and replaced by functionality to create PIT histograms (`pit_histogram_sampel()` and `get_pit_histogram()`)


typo: sampel -> sample

seabbs · 2024-10-22T10:40:20Z

R/get-pit-histogram.R

+#' @examples
+#' library("ggplot2")
+#'
+#' example <- as_forecast_sample(example_sample_continuous)


isn't it already of class sample and so this line isn't needed?

seabbs · 2024-10-22T10:42:43Z

R/metrics-sample.R

-pit_sample <- function(observed,
-                       predicted,
-                       n_replicates = 100) {
+pit_histogram_sample <- function(observed,


noting if/when we get rid of get_ then pit_histogram and pit_histogram_sample are quite hard to distinguish.

seabbs · 2024-10-22T10:43:43Z

inst/manuscript/manuscript.Rmd

-  plot_pit() + 
-  facet_grid(target_type ~ model)
+  get_pit_histogram(by = c("model", "target_type")) |>
+  ggplot(aes(x = mid, y = density)) +


I'd personally prefer we do ship a simpler wrapper around this code (and the labs) to reduce duplication though obviously it isn't very complicated

Do you mean to reinstate plot_pit(), maybe even as S3 function that calls get_pit_histogram() and then plots? Or something else?

seabbs · 2024-10-22T10:44:17Z

inst/manuscript/manuscript.Rmd

  facet_wrap(~ model, nrow = 1) +
-  theme_scoringutils()
+  theme_scoringutils() +


this one uses the scoringutils theme and the other one doesn't

sbfnk and others added 10 commits October 16, 2024 12:03

pit_sample -> pit_sample_histogram

c62c946

get_pit -> get_pit_histogram

a9a6015

update tests

7920aa0

update namespace

1c4fea2

add news item

9b74855

update manuscript

8a206e1

rename f_dash -> f_bar to match docs

08d25a3

minor re-formatting

c2d4e1a

add xlab/ylab in appropriate place

e18231d

Merge branch 'main' into pit-histogram

674fe37

nikosbosse mentioned this pull request Oct 20, 2024

Cosmetic changes to pit histogram #950

Merged

nikosbosse reviewed Oct 20, 2024

View reviewed changes

R/class-forecast-quantile.R Show resolved Hide resolved

nikosbosse and others added 3 commits October 21, 2024 11:39

Some cosmetic changes

d39f6c0

Update R/get-pit-histogram.R

76176d3

Co-authored-by: Sebastian Funk <[email protected]>

address comments on comments without creating more comments

41059a6

sbfnk added 2 commits October 21, 2024 14:39

use safe if

643c297

add plotting examples for pit histogram

ee34257

nikosbosse approved these changes Oct 21, 2024

View reviewed changes

R/get-pit-histogram.R Show resolved Hide resolved

sbfnk merged commit b370557 into main Oct 22, 2024
9 checks passed

sbfnk deleted the pit-histogram branch October 22, 2024 07:20

seabbs reviewed Oct 22, 2024

View reviewed changes

seabbs mentioned this pull request Oct 22, 2024

get_ function names #948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

913: Functions for PIT histograms #949

913: Functions for PIT histograms #949

sbfnk commented Oct 16, 2024 •

edited

Loading

nikosbosse commented Oct 20, 2024

sbfnk commented Oct 21, 2024 •

edited

Loading

nikosbosse commented Oct 21, 2024 •

edited

Loading

sbfnk commented Oct 21, 2024

nikosbosse left a comment

nikosbosse commented Oct 22, 2024

seabbs commented Oct 22, 2024

seabbs Oct 22, 2024

seabbs Oct 22, 2024

seabbs Oct 22, 2024

seabbs Oct 22, 2024

sbfnk Oct 28, 2024

seabbs Oct 22, 2024

913: Functions for PIT histograms #949

913: Functions for PIT histograms #949

Conversation

sbfnk commented Oct 16, 2024 • edited Loading

Description

Checklist

nikosbosse commented Oct 20, 2024

sbfnk commented Oct 21, 2024 • edited Loading

nikosbosse commented Oct 21, 2024 • edited Loading

sbfnk commented Oct 21, 2024

nikosbosse left a comment

Choose a reason for hiding this comment

nikosbosse commented Oct 22, 2024

seabbs commented Oct 22, 2024

seabbs Oct 22, 2024

Choose a reason for hiding this comment

seabbs Oct 22, 2024

Choose a reason for hiding this comment

seabbs Oct 22, 2024

Choose a reason for hiding this comment

seabbs Oct 22, 2024

Choose a reason for hiding this comment

sbfnk Oct 28, 2024

Choose a reason for hiding this comment

seabbs Oct 22, 2024

Choose a reason for hiding this comment

sbfnk commented Oct 16, 2024 •

edited

Loading

sbfnk commented Oct 21, 2024 •

edited

Loading

nikosbosse commented Oct 21, 2024 •

edited

Loading