added feature infos to JSON dump

[StrikerRUS]

Part of #2604.

Not sure about the implementation, maybe it's better to have something like feature_infos_json_?

[guolinke]

if it is only used one-time, I think you can write it directly, without a new function.

[StrikerRUS]

@guolinke Sorry, not sure that fully understood you. Do you mean to write code of bin_info() and feature_infos() with JSON-specific syntax in GBDT::DumpModel?

UPD: BTW, seems that feature_infos_ (feature_infos()) is also used only once during saving a model.

[guolinke]

sorry, i misread it. Is feature_infos_json_ is only used one time, i think current implementation is okay.

[StrikerRUS]

@guolinke OK. One more thing I can suggest is to rename bin_info() to ToString(). Write new function in bin ToJSON(). Then add function bin_mappers() in dataset which will return std::vector<BinMapper>. It will allow to rewrite feature_infos() in the following manner:

...
bufs.push_back(bin_mapper->ToString());
...

and add feature_infos_json() with the following line:

...
bufs.push_back(bin_mapper->ToJSON());
...

This will be consistent with the rest codebase.

[guolinke]

@StrikerRUS yeah, it makes sense.
However, maybe ToString is not a good name for bin_info? I remember bin_info contains the value range for numerical features, and distinct values for categorical features. It is not the string value of BinMapper class.

[StrikerRUS]

@guolinke

However, maybe ToString is not a good name for bin_info?

Agree! WDYT will be better names instead of ToString() and ToJSON()? Maybe bin_info_string() and bin_info_json()?

[StrikerRUS]

@guolinke Can you please help with bin_mappers() function when have time?

[guolinke]

@StrikerRUS did you mean
https://github.com/microsoft/LightGBM/pull/2660/files#diff-0ce9f6b0c2b65569d0099b591a4617c3R585-R596

?

[guolinke]

maybe use a function like std::vector<std::string> BinMapperApply(func) ?
and pass the bin_info_string() and bin_info_json() as arguements.

[StrikerRUS]

@guolinke

did you mean
https://github.com/microsoft/LightGBM/pull/2660/files#diff-0ce9f6b0c2b65569d0099b591a4617c3R585-R596

Yes, exactly!

maybe use a function like std::vectorstd::string BinMapperApply(func) ?
and pass the bin_info_string() and bin_info_json() as arguements.

Great idea (if I understand correctly 😃 )! To be honest, I have no practice in writing cpp code, so I'll be happy to see any decision you think fits here better.

[guolinke]

@StrikerRUS what is the status of this PR?

[StrikerRUS]

@guolinke If you have some time, please implement your idea. Otherwise, I think it's better to close this PR as I'm not sure I'll be able to do it on my own.

[guolinke]

okay, I will implement it when I have time.

[guolinke]

@StrikerRUS when I implement it, I find the template is a better solution.

[guolinke]

it seems this loop causes the fails, @StrikerRUS could you check it ?

[StrikerRUS]

I commented the loop and discovered that the following line causes segfaults:

auto feature_infos_json_objs = train_data_->feature_infos<true>();

UPD: the following (old code for ordinary string format) is also failing with the same error. Maybe a bug was introduced earlier?..

diff --git a/src/boosting/gbdt_model_text.cpp b/src/boosting/gbdt_model_text.cpp
index b1ea0d06..8180bb67 100644
--- a/src/boosting/gbdt_model_text.cpp
+++ b/src/boosting/gbdt_model_text.cpp
@@ -40,18 +40,18 @@ std::string GBDT::DumpModel(int start_iteration, int num_iteration) const {
           << Common::Join(monotone_constraints_, ",") << "]," << '\n';
 
   str_buf << "\"feature_infos\":" << "{";
-  auto feature_infos_json_objs = train_data_->feature_infos<true>();
-  bool first_obj = true;
-  for (size_t i = 0; i < feature_infos_json_objs.size(); ++i) {
-    if (feature_infos_json_objs[i] != "{}") {
-      if (!first_obj) {
-        str_buf << ",";
-      }
-      str_buf << "\"" << feature_names_[i] << "\":";
-      str_buf << feature_infos_json_objs[i];
-      first_obj = false;
-    }
-  }
+  auto feature_infos_json_objs = train_data_->feature_infos<false>();
+  // bool first_obj = true;
+  // for (size_t i = 0; i < feature_infos_json_objs.size(); ++i) {
+  //   if (feature_infos_json_objs[i] != "{}") {
+  //     if (!first_obj) {
+  //       str_buf << ",";
+  //     }
+  //     str_buf << "\"" << feature_names_[i] << "\":";
+  //     str_buf << feature_infos_json_objs[i];
+  //     first_obj = false;
+  //   }
+  // }
   str_buf << "}," << '\n';
 
   str_buf << "\"tree_info\":[";

[guolinke]

I think we cannot dynamically generate the feature_infos_json_objs, as the model could be loaded from file/string. In this case, both used_feature_map_ and 'feature_bin_mappers_` are lost and cause the error.

[guolinke]

@StrikerRUS I propose a new change, you can check it.

[StrikerRUS]

I see... Now feature_infos field is not actually a JSON representation. I think we can get back to my initial implementation then, which parses those strings and constructs valid JSONs.

[guolinke]

Yeah, it would be better

[StrikerRUS]

Done in the latest commit.