[timeseries] Add support for horizon_weight in time series forecasting metrics
#5058
Conversation
| if quantile_levels is None: | ||
| quantile_levels = [0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9] | ||
| self.quantile_levels = sorted(quantile_levels) | ||
| self._learner: TimeSeriesLearner = self._learner_type( | ||
| path_context=self.path, | ||
| eval_metric=eval_metric, | ||
| eval_metric_seasonal_period=eval_metric_seasonal_period, | ||
| horizon_weight=self.horizon_weight, |
There was a problem hiding this comment.
One potential discussion point: Currently we inject the horizon_weight all the way through TimeSeriesPredictor -> TimeSeriesLearner -> TimeSeriesTrainer -> AbstractTimeSeriesModel (same logic as with eval_metric_seasonal_period).
Alternatively, we could "attach" the horizon_weight to the eval_metric object inside the predictor, and then only pass around the eval_metric object. On the one hand, this removed the need to use horizon_weight as a kwarg in all internal methods. On the other hand, we now need to be extra careful that we are actually passing the eval_metric object (a TimeSeriesScorer) and at no point accidentally serialize it as a str (e.g., "WQL") because that will remove the horizon_weight attribute.
There was a problem hiding this comment.
@abdulfatir does this make sense?
There was a problem hiding this comment.
Sorry, I missed your comment somehow. Both approaches make sense, although I agree that passing the horizon_weight so deep is a bit unappetizing.
On the other hand, we now need to be extra careful that we are actually passing the eval_metric object (a TimeSeriesScorer) and at no point accidentally serialize it as a str (e.g., "WQL") because that will remove the horizon_weight attribute.
I think as long as the external API allows user to pass the eval_metric as a string, it should be fine.
Overall, I think that the alternative that you suggested is cleaner in some sense. If you decide to do that instead, happy to review again.
|
Job PR-5058-34a54ac is done. |
|
Job PR-5058-23504a6 is done. |
Issue #, if available: #4852
Description of changes:
horizon_weight: list[float] | Noneto theTimeSeriesPredictorand all forecasting metrics that allows assigning custom weights to each time step in the forecast horizon when computing the metric.[num_items, prediction_length]. We then multiply this array withhorizon_weight.reshape[1, prediction_length], before applying the final aggregation step.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.