[python] max_bin parameter deprecated

[StrikerRUS]

Refer to this thread.

[wxchan]

I think next may not be clear. Can we give a specific version number?

[StrikerRUS]

@wxchan Good idea! What about 2.0.12?

[wxchan]

@StrikerRUS ok. but we might jump some version. @guolinke can confirm this.

[guolinke]

@StrikerRUS we can remove all function arguments in native python interface.
However, it seems is not possible for sklearn interface.
Thus, how should we deal the parameter priority for the function arguments and parameter dict ?

[wxchan]

@guolinke another topic: do you think we can merge booster and _InnerPredictor into one class?

[guolinke]

@wxchan They are designed for decoupling.
If merge booster and predictor, the dataset and booster will have circle reference, since Dataset needs the predictor for the continued train.

[wxchan]

@guolinke maybe we don't assign predictor to dataset? We can store it in Booster because there must be a new Booster when continue training.

[wxchan]

@StrikerRUS I think we can deprecate feature name and category = 'auto', because we don't support category keeping raw input anyway (it must be transferred to int). The extracted feature names always have lowest priority in this case.

[guolinke]

@wxchan it can be. However, you will need to solve the reference dataset problem.

[StrikerRUS]

@guolinke

we can remove all function arguments in native python interface.

I think it's not necessary, because in most other cases None means no use or something similar.
Do you mean something like Dataset(data, params)?

However, it seems is not possible for sklearn interface.

I think it's more not needed than not possible.

Thus, how should we deal the parameter priority for the function arguments and parameter dict ?

In my opinion function arguments have higher priority.

@wxchan I believe you, but I'll read the code about categorical more deeper and will answer you.

[guolinke]

@StrikerRUS
the problem of function arguments have higher priority is, there are default values(not None) for the function argument. As a result, the parameter in dict will be ignored even we didn't set the function arguments.

[StrikerRUS]

@guolinke Yeah, I understand it and think that it's expected behavior. Users must read function signature and params has description that it is other parameters. I suppose, duplicated params in dict should be ignored in any way.

Removing all function arguments will confusing: there are dozens of parameters and it will be very hard for new users to find needed among them. Function arguments is the minimum set of params for which users should pay attention firstly.

[guolinke]

@StrikerRUS okay, however, we should give a warning when met the duplicate.

[StrikerRUS]

@guolinke Yeah, I agree!

[StrikerRUS]

@wxchan

I think we can deprecate feature name and category = 'auto', because we don't support category keeping raw input anyway (it must be transferred to int). The extracted feature names always have lowest priority in this case.

Do you mean that auto actually the same as None?

[StrikerRUS]

We need to replace """some comment""" with # some comment in order to prevent such things:

[StrikerRUS]

In test_consistency we're getting 4 warnings about data in params. Is it OK?
https://ci.appveyor.com/project/guolinke/lightgbm/build/2.0.10.1162/job/5fruxxvpqvhxeref#L2147
https://ci.appveyor.com/project/guolinke/lightgbm/build/2.0.10.1162/job/5fruxxvpqvhxeref#L2157
https://ci.appveyor.com/project/guolinke/lightgbm/build/2.0.10.1162/job/5fruxxvpqvhxeref#L2167
https://ci.appveyor.com/project/guolinke/lightgbm/build/2.0.10.1162/job/5fruxxvpqvhxeref#L2177

[wxchan]

The warning in test_consistency is fine, the params are read from file. I think we can set higher stacklevel in warn.

[wxchan]

The auto is designed for pandas input, it gives user option not to transfer pandas category to int, but actually we don't support the format if not transfer. It results in three possible state: None, auto and some values, make situation too complicated.

[StrikerRUS]

@wxchan I think auto deserves a separate PR.

[wxchan]

@StrikerRUS sure.

[StrikerRUS]

@wxchan I'm afraid I don't not understand about stacklevel. From the description I understand that this parameter could hide the part of stack trace but the warning will be raised anyway.
I tried with stacklevel=2:
https://ci.appveyor.com/project/StrikerRUS/lightgbm/build/2.0.10.517/job/b4avrq8g3ja5c3ae#L2148

[wxchan]

@StrikerRUS it basically means it will show where the function (which causes warning) be called. You can choose a reasonable value you like or leave it default, I tried it before, it looks odd anyway.

[StrikerRUS]

@wxchan I think it's better to leave it default.