-
Notifications
You must be signed in to change notification settings - Fork 36
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
Set up horizon_group for when users want to set groups manually in explain_forecast #433
Set up horizon_group for when users want to set groups manually in explain_forecast #433
Conversation
Thanks, sounds like a nice feature. FYI: Just got accepted on CRAN yesterday: https://cran.r-project.org/web/packages/shapr/index.html, but got some additional (cpp??) warnings that needs to be fixed, so will push a new version when that is fixed in any case. Can then include this work. |
Will do, I also wanted to get the PR up to see the results of the tests as they are broken when I try to run the snapshot tests locally for some reason (null character in something). I will try to fix everything, add a test and docs. Nice! =) |
After some closer investigation I concluded that it would be best to not expose the setting (at least not for now) but rather ensure that Please check if there is anything else needed before merging this. |
R/explain_forecast.R
Outdated
group_forecast_setup <- function (group, horizon_features) { | ||
horizon_group <- vector("list", length(horizon_features)) | ||
new_group <- list() | ||
|
||
for (i in seq_along(group)) { | ||
if (!all(group[[i]] %in% horizon_features[[1]])) { | ||
for (h in seq_along(horizon_group)) { | ||
new_name <- paste0(names(group)[i], ".", h) | ||
new_group[[new_name]] <- group[[i]][group[[i]] %in% horizon_features[[h]]] | ||
horizon_group[[h]] <- c(horizon_group[[h]], new_name) | ||
} | ||
} else { | ||
name <- names(group)[i] | ||
new_group[[name]] <- group[[i]] | ||
for (h in seq_along(horizon_group)) { | ||
horizon_group[[h]] <- c(horizon_group[[h]], name) | ||
} | ||
} | ||
} | ||
return(list(group = new_group, horizon_group = horizon_group)) |
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 quite get the intuition in this function.
It seems to give what it is supposed to, I just don't understand how you set it up and the purpose of the if-else thing.
If one adds a feature name in group which is not in horizon_features it simply ignores it.
Can you try to explain it?
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, the intuition was not too easy to get right immediately when I wrote it, so it makes sense that it is a bit opaque. I will give my best explanation here;
group is the list of groups provided by the user, from this we want to create horizon_groups.
horizon_features contains all features included in each horizon, only exogenous variables will differ between h=1 and h=2,... a higher horizon will always contain the features of a lower.
We go over all groups, if the group
- only contains features which are present in h=1: it means that there are no features that will differ between horizons and we do not need to split this group into multiple new groups (i.e. Temp -> Temp.1, Temp.2). We do however need to refer it in each entry of horizon_group as every horizon should have this group.
- otherwise, we need to split the group into the corresponding groups per horizon, and refer them to the appropriate horizon_groups.
As an example, we have Temp (endogenous) and Wind exogenous. We want to explain the two groups:
Temp = c("Temp.1", "Temp.2"),
Wind = c("Wind.1", "Wind.2", "Wind.F1", "Wind.F2").
Temp does not care about horizon as it just provides two lagged observations, but Wind has F1 and F2 which are the two future observations since it is exogenous, these needs to be put in the new groups Wind.1 and Wind.2, finally horizon_group should refer Temp and Wind.1 for h=1 and Temp and Wind.2 for h=2.
Hope this makes sense! =)
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 I sort of got it, so its fine.
Do you have time to add a quick example to the vignette?
Apart from the above the rest should be fine. It may be nice with an example of how to use it in the vignette, so feel free to add it if you have the time. I merged with the new master, updated test files, docs and linted. |
@jonlachmann I just merged this to push it to CRAN today. If you are up for it later, feel free to add a quick example to the vignette on how to use the new feature. |
No description provided.