tidyverts / fabletools

General fable features useful for extension packages
http://fabletools.tidyverts.org/
89 stars 31 forks source link

Distribution rbind error in reconciliation: dimnames missing #214

Closed FinYang closed 4 years ago

FinYang commented 4 years ago

Hi there,

Reconciliation breaks down if there is a mix of reconciliation model and base model since, probably, 67fb06d7f2b6e6dd9e826ee9d0a1cb3cfeba0ae9.

Here is a minimal example, taken from the documentation for reconcile.

library(fable)
lung_deaths_agg <- as_tsibble(cbind(mdeaths, fdeaths)) %>%
  aggregate_key(key, value = sum(value))

lung_deaths_agg %>%
  model(lm = TSLM(value ~ trend() + season())) %>%
  reconcile(lm = min_trace(lm)) %>%  # This works
  forecast()

The above works while the following does not.

lung_deaths_agg %>%
  model(lm = TSLM(value ~ trend() + season())) %>%
  reconcile(lm2 = min_trace(lm)) %>% # This does not
  forecast()

It gives the error

Error: Distributions must have the same dimnames to be combined.

I did some digging.

temp <- lung_deaths_agg %>%
  model(lm = TSLM(value ~ trend() + season())) %>%
  reconcile(lm2 = min_trace(lm))

Once there are two models called lm and lm2 in temp, the forecast results for them do not have the same dimnames on the distribution.

forecast(temp$lm)[[1]]$value %>% dimnames()
#> [1] "value"
forecast(temp$lm2, key_data=key_data(temp))[[1]]$value %>% dimnames()
#> NULL

The reason is since 67fb06d7f2b6e6dd9e826ee9d0a1cb3cfeba0ae9, the dimnames has been assigned to the forecast, https://github.com/tidyverts/fabletools/blob/7ee7681106554525a5cc6b4697f2e5eb191bfd11/R/forecast.R#L176 which does not get carried over in reconciliation: https://github.com/tidyverts/fabletools/blob/3e39c4c984353b9df552c946afbc6db5c55a3943/R/reconciliation.R#L164-L168 resulting in the error in vctrs::vec_rbind.

Adding some lines to keep the dimnames in forecast.lst.mint.mdl would probably solve everything. I just commented out that line in 67fb06d7f2b6e6dd9e826ee9d0a1cb3cfeba0ae9 as a workaround for now.

Cheers

mitchelloharawild commented 4 years ago

Thanks, I've added the dimnames for the reconciled distributions now.