mlr-org / mlr3temporal

Forecasting for mlr3
https://mlr3temporal.mlr-org.com
GNU Lesser General Public License v3.0
20 stars 2 forks source link

Update `LearnerRegrForecastAverage` to use latest mlr3 API #70

Closed nathaneastwood closed 2 years ago

nathaneastwood commented 2 years ago

I present this PR not as a completed piece of work but as an attempt to start a conversation of how to move this project forwards.

I tried to run the example named "Example 2" presented within ./attic/example.r:

library(mlr3)
library(mlr3temporal)
task = tsk("airpassengers")
learner = LearnerRegrForecastAverage$new()
learner$train(task,row_ids = 1:20)
p = learner$predict(task,row_ids = 21:55)

However the last line does not run since check_prediction_data.PredictionDataForecast does not exist (this is also mentioned in #67).

I worked through the code within mlr3 to see what was required and I came to the following conclusions:

  1. The .predict() methods seem to return a list(), so I adjusted this method in LearnerRegrForecastAverage to do so.

  2. check_prediction_data.PredictionDataForecast() doesn't exist so I added it based upon check_prediction_data.PredictionDataRegr(). This may or may not need adjusting as I suspect a better approach would be to have the PredictionDataForecast S3 class inherit from the PredictionDataRegr S3 class and dispatch to the check_prediction_data.PredictionDataRegr() method instead of having a new method. In the interest of time, I did not investigate where this class is actually generated however as I think this is done dynamically since I couldn't see it anywhere within mlr3 - I may well be incorrect here and I would be interested to know.

  3. In order for check_prediction_data.PredictionDataForecast() to work, I had to add both as_prediction.PredictionDataForecast() and assert_prediction_count(). The former is a new method based upon the as_prediction.PredictionDataRegr() method albeit without the check argument being passed since this caused an error and I did not have time to investigate. assert_prediction_count() is a non-exported function within mlr3 so I suspect a better solution would be to export it there or maybe move it to mlr3misc?

I am of course not close to this codebase and so I have made rather large assumptions based around the design which may invalidate this PR however I would be interested to hear your thoughts regardless on how to take this forwards. I recognise that this package is still very much in development and therefore the API is subject to change. Given that this is not a complete set of work, I did not make any adjustments to tests, nor did I attempt to run R CMD check.

pfistfl commented 2 years ago

This looks good to me. I will merge this now and see if I can get CI running again.

pfistfl commented 2 years ago

To address your points:

  1. absolutely, this is a change made in mlr3 to improve serialization.
  2. I guess @mllg is the correct person to ask this. I will also try to fix this as-is currently, I guess a change would need to be mlr3-wide.
  3. Also directed @mllg I guess. Since it is a relatively small function I guess porting it over for now is fine.
nathaneastwood commented 2 years ago

Thanks @pfistfl. I suspect that similar changes may need to be made to the other learners in this package. Looking at LearnerRegrForecastVAR and LearnerRegrForecastAutoArima for example, they return PredictionForecast objects from the private$.predict() method. I did not run the code for those yet as I wanted to start with the simple Average.

pfistfl commented 2 years ago

I am working on this atm, a few more changes are required so the predictions get assigned the correct PredictionDataForecast class. I'll push an update in a bit.

pfistfl commented 2 years ago

Hey @nathaneastwood I have pushed a working version, all unit tests (except commented out autotests) pass now.

I guess a sensible next step would be to test-ride a little and report errors. Especially some learner implementations could perhaps be improved slightly.

I do have to focus on a few other things now, but would be happy to assist in future things or fix stuff.

nathaneastwood commented 2 years ago

Excellent - thanks a lot @pfistfl! I will take a look at this and see if I come across any additional issues.