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

Variable selection - update all methods #15

Open
wants to merge 30 commits into
base: variable_selection
Choose a base branch
from

Conversation

AniekMarkus
Copy link

  • Methods implemented with the following design principle:

Wrapper – Forward/Backward selection, Boruta -> Feature engineering
Filter – NJMIM, Univariate selection -> Feature engineering
Embedded – LASSO / IHT -> Model development

  • All feature engineering methods combined in one R script VariableSelection.R

@AniekMarkus AniekMarkus changed the base branch from main to variable_selection October 31, 2023 14:16
@AniekMarkus
Copy link
Author

This pull request includes branches:

  • _variable_selection_boruta
  • _variable_selection_iht_cv
    (these can thus be removed)

@AniekMarkus AniekMarkus requested a review from egillax November 2, 2023 10:49
@AniekMarkus
Copy link
Author

This check is failing:

Error (test-featureImportance.R:60:3): feature importance works with logger or without covariates ──
Error in UseMethod("filter"): no applicable method for 'filter' applied to an object of class "NULL"

@egillax
Copy link

egillax commented Nov 7, 2023

I just looked at the failing test. That was because the unvariateFeatureSelection wasn't handling the case correctly when you ask about top K features and there are <= K features in the dataset. In that case it would remove all the data since covariateIdsInclude was NULL. I changed it to keep all data in that case. Let's see if things pass now.

@egillax
Copy link

egillax commented Nov 7, 2023

@AniekMarkus currently this is failing because of warnings in the R check. Generally we want to make sure it passes R check (no errors and warnings) locally before creating a PR.

You can run it locally using devtools devtools::check()

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2023

Codecov Report

Attention: 233 lines in your changes are missing coverage. Please review.

Comparison is base (5c35732) 86.85% compared to head (3ee55bd) 85.80%.

❗ Current head 3ee55bd differs from pull request most recent head e52c65f. Consider uploading reports for the commit e52c65f to get more accurate results

Files Patch % Lines
R/VariableSelection.R 34.14% 135 Missing ⚠️
R/CyclopsModels.R 4.04% 95 Missing ⚠️
R/CyclopsSettings.R 50.00% 3 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           variable_selection      #15      +/-   ##
======================================================
- Coverage               86.85%   85.80%   -1.05%     
======================================================
  Files                      51       50       -1     
  Lines                    9867    10006     +139     
======================================================
+ Hits                     8570     8586      +16     
- Misses                   1297     1420     +123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants