Skip to content

Fix workflowsets imports, zero-coefficient labeling, and feature-cutoff bugs from #31 review - #40

Open
eboyer221 wants to merge 2 commits into
mainfrom
dev-vi-glmnet-fixes
Open

Fix workflowsets imports, zero-coefficient labeling, and feature-cutoff bugs from #31 review#40
eboyer221 wants to merge 2 commits into
mainfrom
dev-vi-glmnet-fixes

Conversation

@eboyer221

Copy link
Copy Markdown
Contributor

What

Addresses code review feedback on #31 (the vip::vi() replacement).
Three fixes, plus one item left as an open question:

  1. Orphaned imports. @importFrom workflowsets extract_fit_parsnip and extract_spec_parsnip are declared but nothing calls them - .viGlmnet() replaced the old workflowsets |> vip::vi() chain with parsnip::extract_fit_engine() directly. Removed both, and removed workflowsets from DESCRIPTION since nothing in the package uses it anymore.

  2. Zero coefficients mislabeled. .viGlmnet()'s Sign = ifelse(coefs > 0, "POS", "NEG") labeled an exact-zero coefficient "NEG" - indistinguishable from an actually-negative one. This matters for LASSO/elastic-net models, where zeroing out a coefficient is the normal way a feature gets excluded. Now labeled NA.

  3. Last feature silently dropped. extractTopFeats()'s default prop_vi_top_feats = c(0, 1) is documented to return all features but filter(cum_imp < cum_vi_upper & ...) used a strict <, and the least-important feature's cumulative importance is exactly equal to the total - so it always got excluded. Changed to <=.

Left open

Whether .viGlmnet() should use the tuned penalty (as the multi-class path in extractTopFeats() already does) instead of min(glmnet_fit$lambda) - flagging for @AbhirupaGhosh.

Testing

Added two tests to tests/testthat/test-core-ml.R. I confirmed both catch the original bugs by temporarily reverting the fix and re-running: the zero-coefficient test failed as expected, and testing against a real fitted model reproduced the exact symptom described - the least important feature (feat_noise_1) silently missing from the result. Full suite passes (214/214).

@amcim amcim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall these changes look pretty good. I dealt with the merge conflict with the test file (did not require any real changes) by merging main into the branch, and I removed some more stale imports.

Some notes on the changes. In the original implementation of this in PR #31 , I aimed to recreate the results of VIP exactly. For (2), this is a good change. I copied how VIP did it, but its better this way and I approve of it.

Additionally for the open question here, the mismatch existed before PR #31. What this means is that the call to vip never used the tuned penalty. So if its changed to match, that would be changing the functionality.

For (3), I tested using demo_fit.rda data. With this change, its not just one missing feature that is kept. It also keeps features that have an importance score of 0. If we want to exclude the 0s we will need to add to the filter.

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

Successfully merging this pull request may close these issues.

2 participants