Reduce package NAMESPACE

[nikosbosse]

I believe scoringutils NAMESPACE is much larger than what it should be, which makes it difficult to approach and maintain. We probably need to have a longer discussion about this but some early thoughts:

• should the low-level functions really be exported? There is no workflow in the vignettes presenting when it would make sense to use them directly.
• [x] functions such as run_safely() should definitely not be exported IMO since they don’t fit with scoringutils core functionality. Instead, we should encourage users to use the tools they are already familiar for this kind of work beyond scoringutils scope, such as tryCatch() or purrr::safely().
• [x] assert_forecast_generic() is marked as internal, but also exported

Originally posted by @Bisaloo in https://github.com/epiforecasts/scoringutils/pull/791#pullrequestreview-2003401135

[seabbs]

The last two definitely shouldn't be exported.

should the low-level functions really be exported

Its not totally clear to me what we are talking about here. If we don't think there is a use case that is immediately obvious then lets make them internal

[Bisaloo]

When I say low-level functions, I am talking about the function listed in the Lower-level scoring functions.

It seemed initially like an interesting idea, and a good way to target different user persona & needs, but with the new class approach, it seems scoringutils is increasingly moving towards an opinionated happy path, in which the low level scoring functions don't seem to fit very well.

[seabbs]

Right got you.

So I think we currently use these in the standard default metrics lists (i.e https://epiforecasts.io/scoringutils/dev/reference/metrics_binary.html?q=metrics_binary#ref-usage) but that we also imagine people making their own lists from scratch using these functions (i.e not adapting the default metrics) if they know what they want.

To me that seems part of the same user pathway and so would justify keeping all metrics that we would expected to be used in that way as exported. It would also imply if we have any other metrics (i.e that might be wrapped by one of these) then it would be internal.

@Bisaloo do you agree or do you also think these should be internal and we instead just depend on the lists of functions (which is currently documented as the main/only use of package? I can see argument that anything else represents a different pathway but to me the default lists are just helpers.

[Bisaloo]

To me that seems part of the same user pathway and so would justify keeping all metrics that we would expected to be used in that way as exported. It would also imply if we have any other metrics (i.e that might be wrapped by one of these) then it would be internal.

Worth double checking but I think it should still work the same if the functions are kept internal.

@Bisaloo do you agree or do you also think these should be internal and we instead just depend on the lists of functions (which is currently documented as the main/only use of package? I can see argument that anything else represents a different pathway but to me the default lists are just helpers.

I still would probably prefer a bit that all the low-level metric functions are internal but it's more of a subjective preference than for the other recommendations in this thread so happy to leave the final choice to you.

[seabbs]

Worth double checking but I think it should still work the same if the functions are kept internal.

It would all the work the same apart from making your own list from scratch - that is the only trade-off.

it's more of a subjective preference than for the other recommendations in this thread so happy to leave the final choice to you

Given this shall we split it out into its own issue and action there? Personally, I am leaning towards leaving as is but maybe sorting out the other exports will help clarify things

[nikosbosse]

Re assert_forecast_generic(): The reason why it is exported at the moment is because users who want to extend the current S3 classes might want to use it. When moving to an abstract forecast S3 class we might be able to call some assert_forecast.forecast before calling the more specific versions for binary forecasts or something clever like that.

[nikosbosse]

I'm also leaning towards keeping the scoring functions as exports, but happy to discuss in a separate issues.

@Bisaloo do you have thoughts on other functions that should be made internal? run_safely() is internal now and assert_forecast_generic() can probable be dealt with as a method.

[seabbs]

Re assert_forecast_generic(): The reason why it is exported at the moment is because users who want to extend the current S3 classes might want to use it. W

I'm leaning towards internal for now and gauge user demand heree

[Bisaloo]

The last case I see would be sample_to_quantile() (replaced by a S3 method) but I think this is already tracked somewhere else

[nikosbosse]

The two functions we discussed are internals now. sample_to_quantile() is its own issue (#851) and I'd vote to keep the scoring functions exported, given that I think this may be useful to others who want to use these functions directly. Closing this issue now, but please feel free to reopen it.