Plot s3 methods

[nikosbosse]

This PR adds S3 methods for

• plot_correlation() (replacing plot_correlation() by an S3 method for plot())
• plot_pairwise_comparison()

I'm not entirely sure how to document these methods properly. By default, the methods are hidden so a user wouldn't really know how to access the documentation.

Update:

I added links / info to the base function via @Seealso. So e.g. the documentation to pairwise_comparison() has a link to plot.pairwise_comparison().

Outdated:

At the moment I added the following to plot.R:

#' @title Various Plotting Methods for Outputs of scoringutils Functions
#' @param x S3 object to be plotted, as produced by various scoringutils
#' functions.
#' @param ... other arguments
#' @export
plot <- function(x, ...) {
  UseMethod("plot")
}

And then to every S3 method, I added #' @rdname plot. The problem with this is that all the documentation just gets thrown together like this:

In particular, all the function arguments are documented together:

Maybe it's not such a problem after all and we just have to standardise arguments a bit more and make it clearer overall?

Or do you have a different idea?

[seabbs]

So as plot is already a generic you shouldn't need to remake it and in fact you definitely don't want to. I think the difficulty finding the docs is sadly just part of the cost of doing business but there might be nicer solutions.

[nikosbosse]

ayeaye. I did some changes in the last commit. Now there is no plot() generic exported and I'm not combining any documents. Instead I added links / info to the base function via @Seealso. So e.g. the documentation to pairwise_comparison() has a link to plot.pairwise_comparison(). Haven't really found a better solution unfortunately.

[seabbs]

Love it. Can also use the family approach as well to get all the plot functions to link out to each other if you wish

[nikosbosse]

Yes! Would you add all plotting functions or just the S3 methods?

[seabbs]

could add all plotting functions with the aim of going fully S3 at some point? Note the downside of this approach is that it introduces PR noise when you add a new plot function (as all the other docs will auto update to include it)

[nikosbosse]

I think this is good to go for another look

[seabbs]

LGTM

[seabbs]

Where are we on this one?

[nikosbosse]

I think it's in principle good to go. But since there are a few conflicts anyway my idea was to fix all of the other stuff first (so that we have more clarity on what classes will exist) and do the plotting functions last. Overview of other to dos: scoringutils 2.0 (view)

[seabbs]

fair enough sounds good

[seabbs]

what are the blockers here?

[nikosbosse]

• The PR needs a bit of a polish (e.g. removing outdated files)

Only blocker I see is naming conventions: As discussed in #511, we maybe want to have some consistent naming convention for the classes that are only used for plotting.

I was also briefly considering whether there was a case to rename correlation() to get_correlation(). My next two thoughts were

• hm maybe that doesn't quite fit, because we call correlation() on the scores rather the raw forecasts, whereas with the other get_-functions we mostly use them on the forecasts (although e.g. get_forecast_unit() should be able to do both)
• it seems useful to write a up a document with a consistent naming scheme for things that we can refer to

[nikosbosse]

First draft: #520

[sbfnk]

Maybe it's not such a problem after all and we just have to standardise arguments a bit more and make it clearer overall?

I think this is generally how SD methods are documented. You have some influence over the order in which things appear etc., see https://cran.r-project.org/web/packages/roxygen2/vignettes/reuse.html

[seabbs]

So my takeaway from our chat earlier was that we may have been wrong about this for functions outside of the main score workflow and so we should stick with named plotting functions?

[nikosbosse]

I'm happy with named plotting functions for now. One JSS reviewer wanted plotting methods, but I think we can make a good case against that. Closing this PR, but please feel free to reopen.