[MRG] check duplicate params #961

wxchan · 2017-10-03T15:52:43Z

check params which are alias to each other and print warnings
add get_parameter function to let user check current parameters, easy to debug

guolinke · 2017-10-04T10:02:31Z

@wxchan I think this solution is too heavy.
If we want to add a new parameter, it needs to edit many places.

Maybe we can refactor the whole config class, for example, we can put all parameters into a unordered_map, and retrieve the value by using its names.

StrikerRUS · 2017-10-04T10:20:33Z

python-package/lightgbm/basic.py

@@ -1397,6 +1397,50 @@ def reset_parameter(self, params):
                self.handle,
                c_str(params_str)))

+    def get_parameter(self, is_output_raw=False):


I think that get_parameter in python API is needed anyway, but please update docstring with the aim of uniformity with other docstrings.

"""Get parameters of Booster. Parameters ---------- is_output_raw : bool, optional (default=False) Whether to output raw string. Returns ------- param : dict or string Parameters of Booster. If ``is_output_raw=False``, dictionary of parameters with string key and string value; otherwise, string of parameters as concat of "key:value". """

I already revert this. Looking for better solution.

wxchan · 2017-10-04T14:45:52Z

@guolinke I agree. But what's your solution to save different types of values in unordered_map, a common way for me is std::variant, but it's not supported until c++17.

wxchan · 2017-10-04T14:48:23Z

@guolinke btw, there may be some bug in python categorical feature, is Dataset and Booster using the same Overall Config in c_api now?

guolinke · 2017-10-05T01:53:15Z

@wxchan there using the different configs.

BTW, we can define a class, e.g.
class ParamVal{
int int_val;
double double_val;
bool bool_val;
std::string str_val;
std::vector double_arr_val;
...
}

And we can parse all these types when read a parameter, and retrieve the type we need.

guolinke · 2017-10-05T02:06:36Z

@wxchan another solution is using multi maps.

for example,

unordered_map::<string, int> int_map;
unordered_map::<string, double> double_map;
...

and using another map to check its types.

However, my concern is such a dynamic solution may cause the performance issue.

Can we simplify the GetParam and make it automatically ?

wxchan · 2017-10-05T05:11:56Z

@guolinke one possible solution is to save params to a map in GetInt/GetDouble/GetBool/GetString or KeyAliasTransform. I can revert get_parameter if there is no good solutions now.

wxchan · 2017-10-05T05:13:24Z

@guolinke if they are using overall config, when using get_parameter, I can only get parameters from overall config of Booster? How can I know whether overall config of Dataset is working?

guolinke · 2017-10-05T10:35:40Z

@wxchan currently, Dataset class doesn't store the parameters. The parameters are used temporarily when constructing Dataset.

wxchan · 2017-10-05T11:52:40Z

@guolinke so with get_parameter, the result might not be the actual parameters used for datasets?

guolinke · 2017-10-05T14:53:38Z

@wxchan yeah. So you may need to store a IOConfig in the Dataset class.

clean codes

This reverts commit ec1d8dd.

wxchan · 2017-10-05T15:23:31Z

@guolinke so how can I quickly confirm whether the categorical feature setting works? like in #893 . I think current logic should be right, though it's a little messy.

wxchan · 2017-10-05T15:23:59Z

@guolinke I revert get_parameter first, you can review other part.

guolinke · 2017-10-05T15:37:31Z

@wxchan
In cpp side, the feature indices of the categorical features could be restored by using the bin_mappers and feature indices mappers.
in python side, the parameter of categorical features could be stored in the Dataset, and using that to check the settings.

guolinke · 2017-10-05T15:39:58Z

include/LightGBM/config.h

+      if (alias != alias_table.end()) { // found alias
+        auto alias_set = tmp_map.find(alias->second); 
+        if (alias_set != tmp_map.end()) { // alias already set
+          // set priority alphabetically to ensure reproducible behavior


maybe using the strlen for the priority is better? since users often use the short names.

guolinke · 2017-10-06T11:46:42Z

include/LightGBM/config.h

@@ -81,6 +81,7 @@ struct ConfigBase {
    const std::unordered_map<std::string, std::string>& params,
    const std::string& name, bool* out);

+  static void KVIntoMap(std::unordered_map<std::string, std::string>& params, const char* kv);


* add get params clean codes * check duplicate params * Revert "add get params" This reverts commit ec1d8dd. * set priority by length & check duplicate * rename function

wxchan requested a review from guolinke October 3, 2017 15:52

StrikerRUS mentioned this pull request Oct 4, 2017

[python] sorting parameters #953

Closed

StrikerRUS reviewed Oct 4, 2017

View reviewed changes

wxchan added 3 commits October 5, 2017 23:19

check duplicate params

166bbbe

add get params

ec1d8dd

clean codes

Revert "add get params"

087fe99

This reverts commit ec1d8dd.

wxchan force-pushed the param branch from 40e6e52 to 087fe99 Compare October 5, 2017 15:20

Merge branch 'master' into param

a6b4bb1

wxchan changed the title ~~[WIP] check duplicate params & add get_parameter~~ [MRG] check duplicate params Oct 5, 2017

Merge branch 'master' into param

9c001fb

guolinke reviewed Oct 5, 2017

View reviewed changes

guolinke and others added 3 commits October 6, 2017 15:43

Merge branch 'master' into param

ca7ee12

set priority by length & check duplicate

e4fa074

Merge branch 'param' of https://github.com/wxchan/LightGBM into param

fb84bac

guolinke reviewed Oct 6, 2017

View reviewed changes

rename function

510a9c0

guolinke approved these changes Oct 6, 2017

View reviewed changes

Merge branch 'master' into param

4b34c9f

guolinke merged commit d3e88ef into microsoft:master Oct 7, 2017

wxchan deleted the param branch October 7, 2017 09:27

guolinke pushed a commit that referenced this pull request Oct 9, 2017

[MRG] check duplicate params (#961)

3f0155b

* add get params clean codes * check duplicate params * Revert "add get params" This reverts commit ec1d8dd. * set priority by length & check duplicate * rename function

lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] check duplicate params #961

[MRG] check duplicate params #961

wxchan commented Oct 3, 2017

guolinke commented Oct 4, 2017

StrikerRUS Oct 4, 2017 •

edited

Loading

wxchan Oct 5, 2017

wxchan commented Oct 4, 2017

wxchan commented Oct 4, 2017

guolinke commented Oct 5, 2017 •

edited

Loading

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

guolinke Oct 5, 2017

guolinke Oct 6, 2017

[MRG] check duplicate params #961

[MRG] check duplicate params #961

Conversation

wxchan commented Oct 3, 2017

guolinke commented Oct 4, 2017

StrikerRUS Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

wxchan Oct 5, 2017

Choose a reason for hiding this comment

wxchan commented Oct 4, 2017

wxchan commented Oct 4, 2017

guolinke commented Oct 5, 2017 • edited Loading

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

wxchan commented Oct 5, 2017

wxchan commented Oct 5, 2017

guolinke commented Oct 5, 2017

guolinke Oct 5, 2017

Choose a reason for hiding this comment

guolinke Oct 6, 2017

Choose a reason for hiding this comment

StrikerRUS Oct 4, 2017 •

edited

Loading

guolinke commented Oct 5, 2017 •

edited

Loading