Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 432: Add some basic functions for as_point and as_quantile #790
Issue 432: Add some basic functions for as_point and as_quantile #790
Changes from 8 commits
a9c180e
bc21d3e
bb3c8dd
c5cf871
e64aae9
112fe32
e4a43c7
dd23db3
82fd28d
0791871
e13dfd0
6fd8962
90d17a4
82a36f3
6be7724
737f4e9
5fae245
c2b62f3
6688f2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor comment is that this feels as if there is a new
point
, orquantile
class. I believe it would make sense, and be slightly easier to wrap my head around as a potential user, if we re-used the existing classes and terminology. I.e.,forecast_point
andforecast_quantile
.This would lead to longer names but that's probably still ok 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think if we are going with a abstract class as in #373 we should rename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprised to see this as an argument - do you have a use case in mind where one would want it not to be 0.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not reallly but I thought there was little issue with giving this option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that it's a minor issue but it does add to the cognitive burden on anyone coming tot he function so if we think there's no use case it might be better to simplify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we validate the forecast at all then we should also make sure the forecast type is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seee above point and my disagreement with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to explicitly check for the correct forecast type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't thinks its needeed given it dispatches based on the type in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we do this with other auto dispatch functions? If not I think its a new issue and something to implement package widee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't do this elsewhere so suggest it as a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new issue sounds good. If we're keeping the
assert_forecast()
then I think we definitely want it here.The bigger question is probably "do we want to do a validation check at all?" and I think that's up for debate.
But if we're doing a validation check then we should also check whether the object has the right forecast type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I agree.
as_point.forecast_sample
isn't a user facing function and so they should only be callingas_point
hence specifying the class again is a not needed. I can see the safety argument but it also bloats the code and makes it hard to ever rely on 3 properly with that pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for calling this
quantile_level
for two reasons:quantile_level
everywhere elsequantile_level
I see the point for using the plural, but overall think it would be easier to use the singular here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree but happy to standardise with the rest of the code base. It feels like this needs a new issue though as its not very intuitive. (i.e its probs not prob in
quantile
for a reason).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could just call
as_forecast()
instead. The currentsample_to_quantile()
function does that.I'd be happy to avoid the additional code complexity of introducing a new function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do this hence the new checking option and it still wouldn't work. This what motivated the question about refactoring
as_forecast
as the current approach with the type specific checking really doesn't seem feasible for future extensions.I think we should make a new issue for this (related to the
as_forecast
issue). There is also a more general point about whether or notas_forecast
should be able to reclass aforecast
object or whether it does actually make sense workflow wise to have something that explicitly removes and then readds the class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new_forecast()
callsas.data.table()
which strips the class anywayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and was having some weird errors. Can you confirm? More generally I think its helpful to make this explicit in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'll check it out and play around a bit with this. Circling back!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we strictly need an additional argument given that users can specify the metrics in
score()
.If we decide to have it then I'd vote for making the default
metrics = get_metrics(scores)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
get_metric
do if the input isn't a score object? That was the rub here.I only added this so I can could use
summarise_scores
on more general objects and save introducing more code.More generally I also quite like the idea that naive users can reduce their score object at this point as many likely won't know to manipulate the metric list going into score regardless of docs.
All of above being said I think this is a new issue to discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense not to use
summarise_scores()
here (at least for now).summarise_scores()
ultimately is a glorified one-liner and the actual work happens here:Then we could discuss the
metrics
argument in a separate issue/PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure