Issues with new parsnip

[topepo]

My local revdep checks did not see an issue but CRAN flagged an error from the new version of parsnip.

══ Testing test-panel-data.R ═══════════════════════════════════════════════════
[ FAIL 2 | WARN 0 | SKIP 0 | PASS 33 ]

── Failure (test-panel-data.R:74:5): ensemble_average(): Forecast Jumbled ──────
accuracy_tbl$mae < 500 is not TRUE

`actual`:   FALSE
`expected`: TRUE 

── Failure (test-panel-data.R:136:5): ensemble_weighted(): Forecast Jumbled ────
accuracy_tbl$mae < 400 is not TRUE

`actual`:   FALSE
`expected`: TRUE 

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 33 ]

The issue is related to the change in the xgboost parameter in tidymodels/parsnip#499

The test case uses prophet_boost and boost_tree models. The latter has the same results; I think that it doesn't tune over mtry. The prophet model is different, due to the mtry difference. Partial waldo diffs are:

old$fit$fit$models$model_2$call vs new$fit$fit$models$model_2$call
  `xgboost::xgb.train(params = list(eta = 0.3, max_depth = 6, gamma = 0, `
- `    colsample_bytree = 1, min_child_weight = 1, subsample = 1, `
+ `    colsample_bytree = 0.0588235294117647, colsample_bynode = 1, `
- `    objective = "reg:squarederror"), data = x$data, nrounds = 15, `
+ `    min_child_weight = 1, subsample = 1, objective = "reg:squarederror"), `
- `    watchlist = x$watchlist, verbose = 0, nthread = 1)`
+ `    data = x$data, nrounds = 15, watchlist = x$watchlist, verbose = 0, `
+ `    nthread = 1)`

`old$fit$fit$models$model_2$params` is length 9
`new$fit$fit$models$model_2$params` is length 10

    names(old$fit$fit$models$model_2$params) | names(new$fit$fit$models$model_2$params)    
[2] "max_depth"                              | "max_depth"                              [2]
[3] "gamma"                                  | "gamma"                                  [3]
[4] "colsample_bytree"                       | "colsample_bytree"                       [4]
                                             - "colsample_bynode"                       [5]
[5] "min_child_weight"                       | "min_child_weight"                       [6]
[6] "subsample"                              | "subsample"                              [7]
[7] "objective"                              | "objective"                              [8]

`old$fit$fit$models$model_2$params$colsample_bytree`: 1.0
`new$fit$fit$models$model_2$params$colsample_bytree`: 0.1

`old$fit$fit$models$model_2$params$colsample_bynode` is absent
`new$fit$fit$models$model_2$params$colsample_bynode` is a double vector (1)

I'm not sure what to do about this since the new mapping of mtry to colsample_bynode is more appropriate (that xgboost argument didn't exist when parsnip was written).

We could change the call to prophet_boost and tune over mtry. I think that prophet_boost() needs to be updated anyway to be consistent with the new mapping and let users use

set_engine("prophet_xgboost", colsample_bytree = tune()))

if they want.

[mdancho84]

Let me run some things on my end to double check. I have also just released a version of modeltime 0.6.0 to CRAN, but they have not yet approved. So I'm going to tell them to hold off.

[topepo]

Should I wait on the parsnip release?

[mdancho84]

I'm taking a look now. Hang tight.

[mdancho84]

Functionally everything is still working - The test that is failing is an accuracy measurement. What's weird is that the new default parameters seem to make the accuracy worse.

So it seems like xgboost is working nicely, and I just need to address changes in modeltime::prophet_boost() and modeltime::arima_boost() to make sure my algorithms perform well.

What changes were made that I need to adjust? Any pointers?

[mdancho84]

Looks like my colsample_bytree is now getting set to a very low value. Should be 1.

[mdancho84]

I think I see what's going on. If I enter a value of colsample_bytree = 1, then it converts it to a percentage of the columns, which is probably not what we want to do. https://github.com/tidymodels/parsnip/blob/6f5ed7e2510df041921f1133f393587f8f83d3a6/R/boost_tree.R#L345

[mdancho84]

So for backwards compatibility, I'd handle a value of strictly 1 separately, otherwise you're going to blow up a lot of models.

[topepo]

Yes. I figured that it was related to the colsample_* switch but that makes sense.

The old approach would preclude anyone from using a value of colsample_* that would be a count of one. Since xgboost is the only model that parameterizes mtry as a proportion, our models assume that it is a count.

One way around this is to pass an engine argument of counts = FALSE (meaning that your arguments are proportions).

[topepo]

I can wait a bit for the release if that helps

[mdancho84]

It's OK then - It seems that you've thought everything through. I'm switching modeltime over to match the new parsnip, so the sooner you release, the sooner I can release and everything is back to hunky-dory. Just

[mdancho84]

This is taken care of. Thanks for the heads up @topepo