search

epiforecasts / scoringutils

Utilities for Scoring and Assessing Predictions
https://epiforecasts.io/scoringutils/
Other
47 stars 19 forks source link

Reduce number of `try()`, `suppressMessages()`, `suppressWarnings()` used in the package #823

Open nikosbosse opened 1 month ago

nikosbosse commented 1 month ago

scoringutils codebase contains a lot of try(), suppressMessages(), suppressWarnings(). This is likely the symptoms that a refactoring or redesign is necessary. In particular, one potential cause can be the lack of delineation between internal and external functions, as discussed in the earlier NAMESPACE section.

try(), suppressMessages(), suppressWarnings() should usually be a last resort, and not a common pattern in the codebase. Since they affect all conditions, not just conditions created by scoringutils, they can also hide deeper issues, and make debugging harder.

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

seabbs commented 1 month ago

I think the issue that needs addressing here is mainly around the validation of forecast objects and verbosity of that validation?

One solution which I think @Bisaloo is getting at is have fewer user facing functions -> fewer functions that need to validate -> fewer suppressed messages etc.

The other thing that comes to mind is the current desire for score to warn but not fail when an underlying metric fails (as this could be very annoying to users). Potentially we are just happy with it being annoying to users and so should drop? The downside is it may be somewhat unclear to users what has failed

What other issues are there? I think this could do with some detailed feedback?