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

added feature importance to JSON dump #2656

Merged
merged 3 commits into from
Jan 2, 2020
Merged

added feature importance to JSON dump #2656

merged 3 commits into from
Jan 2, 2020

Conversation

StrikerRUS
Copy link
Collaborator

Part of #2604.

Implementation is copied from SaveModelToString without sorting.

@StrikerRUS StrikerRUS merged commit edb9149 into master Jan 2, 2020
@StrikerRUS StrikerRUS deleted the feat_importance branch January 2, 2020 14:19
@StrikerRUS
Copy link
Collaborator Author

@guolinke WDYT about allowing to specify feature importance type in dump / save function? Seems that it will break the existing compatibility for C API.

@guolinke
Copy link
Collaborator

guolinke commented Jan 3, 2020

@StrikerRUS could you explain it with more details?

@StrikerRUS
Copy link
Collaborator Author

@guolinke I mean, at present, feature importance is always dumped with importance_type=0:

std::vector<double> feature_importances = FeatureImportance(num_iteration, 0);

std::vector<double> feature_importances = FeatureImportance(num_iteration, 0);

Will be it useful to allow to specify importance type during saving / dumping model?

@guolinke
Copy link
Collaborator

guolinke commented Jan 3, 2020

As we have standalone FeatureImportance API

LightGBM/src/c_api.cpp

Lines 1563 to 1574 in 516bd37

int LGBM_BoosterFeatureImportance(BoosterHandle handle,
int num_iteration,
int importance_type,
double* out_results) {
API_BEGIN();
Booster* ref_booster = reinterpret_cast<Booster*>(handle);
std::vector<double> feature_importances = ref_booster->FeatureImportance(num_iteration, importance_type);
for (size_t i = 0; i < feature_importances.size(); ++i) {
(out_results)[i] = feature_importances[i];
}
API_END();
}

I think adding importance_type to dump/save is not very necessary.

@StrikerRUS
Copy link
Collaborator Author

Agree! Especially, given that it requires C API breakage.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
@StrikerRUS
Copy link
Collaborator Author

WDYT about allowing to specify feature importance type in dump / save function? Seems that it will break the existing compatibility for C API.

Implemented in #3220.

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

Successfully merging this pull request may close these issues.

2 participants