search

epiforecasts / scoringutils

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

Make `quantile_to_interval` require a `forecast` object #771

Closed nikosbosse closed 3 months ago

nikosbosse commented 4 months ago

And maybe also think about whether that function should be exported at all... I think the main reason it's exported is because it's an S3 method.

nikosbosse commented 4 months ago

ok even after some googling around I'm not sure how to not export an S3 method. @seabbs, you mentioned this link https://stackoverflow.com/questions/68515108/internal-s3-generics-with-an-lapply. The function is supposed to be only used in scripts, not packages. Do you know how to achieve this for packages?

~Scratch that - apparently just removing the export statements does work. What I was seeing was an unrelated error.~

ok it doesn't work right away... There are some weird issues. If I call a function that calls quantile_to_interval() directly, then I get an error. If I run other functions/other tests first, then everything works fine.

nikosbosse commented 4 months ago

So these are essentially two issues:

  1. find a way to not export this function
  2. make it require a validated forecast object
seabbs commented 4 months ago

As this is an internal function I am really not sure we need to worry so much? Why not just not use s3 here? Also as its internal hasn't the input already always been check to make sure its a forecast object?

I would just simplify here as there doesn't seem a lot of reason to worry about this

nikosbosse commented 4 months ago

Well currently it's not an internal function as I don't know how to prevent it from being exported

seabbs commented 4 months ago

but we have agreed it should be? The solution is just not have it be s3 and make it two functions

nikosbosse commented 4 months ago

Makes sense to me - I would personally vote for having it be an internal function as we aren't using the interval format anymore for anything else that's user-facing. What do you think?

seabbs commented 4 months ago

Yes I agree