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

Splits feature_combinations into multiple functions #70

Merged
merged 24 commits into from Aug 19, 2019

Conversation

nikolase90
Copy link
Collaborator

@nikolase90 nikolase90 commented Jul 8, 2019

Summarized

  • Extract parts of feature_combinations into two new functions called feature_exact and feature_not_exact. All the functions are stored in R/features.R
  • Removes observation_weights since it is not used anywhere in the codebase. The function was previously stored in R/shapley.R.
  • Updates shapley_weigths so that it handle cases with zero features and m features, i.e. where s = 0 or s = m.
  • Moves sampling of features into sampling_features_cpp. This part will only be used if exact = FALSE in feature_combinations().
  • Adds tests for functions in R/features.R. See tests/testthat/test-features.R.

Fixes #71 and #72. These issues came up when writing the tests.

Benchmarks

Ran the following chunk using the version from master and nikolai/tests_features

new <- bench::mark(
  new = feature_combinations(m = 15, exact = FALSE, noSamp = 1e4), 
  iterations = 50, 
  filter_gc = FALSE
)
new <- data.table::data.table(new)
knitr::kable(new[, .SD, .SDcols = 1:10])

Current

|expression |   min|  mean| median|   max|  itr/sec| mem_alloc| n_gc| n_itr| total_time|
|:----------|-----:|-----:|------:|-----:|--------:|---------:|----:|-----:|----------:|
|new        | 129ms| 149ms|  144ms| 365ms| 6.704927|        0B|   19|    50|      7.46s|

Master

|expression |   min|  mean| median|   max| itr/sec| mem_alloc| n_gc| n_itr| total_time|
|:----------|-----:|-----:|------:|-----:|-------:|---------:|----:|-----:|----------:|
|old        | 652ms| 770ms|  751ms| 1.03s| 1.29869|        0B|  185|    50|      38.5s|

This was referenced Jul 9, 2019
@nikolase90 nikolase90 requested a review from martinju July 10, 2019 08:28
@nikolase90 nikolase90 mentioned this pull request Jul 10, 2019
15 tasks
@nikolase90 nikolase90 merged commit f38bf2e into master Aug 19, 2019
@nikolase90 nikolase90 deleted the nikolai/tests_features branch August 19, 2019 13:19
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.

replace does not exists
2 participants