koalaverse/vip

Partial matching error with glmnet package

bgreenwell opened this issue · 4 comments

T-Flet commented

Any news on this?
As a sanity check, this is what it should be doing, right?

vi_glmnet <- function(model_fit, lambda) {
    cs <- coef(model_fit$fit, s = lambda)
    tibble(Variable = rownames(cs), Importance = abs(as.vector(cs)), Sign = if_else(as.vector(cs) >= 0, 'POS', 'NEG')) %>%
      arrange(desc(Importance))
}

(vi_model documentation: "Similar to (generalized) linear models, the absolute value of the coefficients are returned for a specific model.")

bgreenwell commented

Yes (although I think lambda should default to NULL). I just haven't found the time to push a fix yet. Happy to accept a PR? Otherwise, I can plan to add the fix early next week?

T-Flet commented

I had a look at the vi and vi_model code, and while the latter only needs adding abs in the glmnet varieties' Importance columns (and perhaps mentioning in the documentation at the top that s is the appropriate parameter name for consistency with coef.glmnet), the required fix for the former seems to be much broader.

I am not an experienced R developer, and what I had in mind was something starting with

sc <- sys.call()
dots_vars <- setdiff(names(sc), c('', names(environment())))

to get ... variables, then using sc$ARG_NAME instead of any bare argument use, and ending by replacing the vi_model etc calls with do.call(vi_*, NEW_DOTS_VARS), but the situation is greatly complicated by non-... arguments having been overwritten by ... ones due to partial matching.

Looking online I found this helper function, but I do not know what scope of changes you are interested in making (trying to address all possible ... shenanigans vs fixing only this s-glmnet case).

I would much rather leave this to you to sort out, as you say, next week (and then see how you did it).

bgreenwell commented

Hey @T-Flet this should be fixed in the current development version if you want to check it out!

  • 👍1