Skip to content
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

Partial matching error with glmnet package #103

Closed
bgreenwell opened this issue May 28, 2020 · 4 comments
Closed

Partial matching error with glmnet package #103

bgreenwell opened this issue May 28, 2020 · 4 comments

Comments

@bgreenwell
Copy link
Member

https://stackoverflow.com/questions/61606337/computing-importance-measure-using-vip-package-on-a-parsnip-model/62064736#62064736

@T-Flet
Copy link

T-Flet commented Oct 5, 2020

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
Copy link
Member Author

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
Copy link

T-Flet commented Oct 6, 2020

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
Copy link
Member Author

bgreenwell commented Oct 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants