trees and forests min_impurity_split deprecation #12240
Conversation
|
This pull request introduces 3 alerts when merging 6aa8807 into 59b15c5 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
2ccaf85 to
5b7ad42
Compare
|
This deprecation is also broken because not specifying a value will lead to a change in behavior between versions :-/ |
|
This pull request introduces 2 alerts when merging 5b7ad42 into dfd009d - view on LGTM.com new alerts:
Comment posted by LGTM.com |
| RandomForestRegressor(bootstrap=True, criterion='mse', max_depth=2, | ||
| max_features='auto', max_leaf_nodes=None, | ||
| min_impurity_decrease=0.0, min_impurity_split=None, | ||
| min_impurity_decrease=0.0, | ||
| min_samples_leaf=1, min_samples_split=2, | ||
| min_weight_fraction_leaf=0.0, n_estimators=100, n_jobs=None, | ||
| oob_score=False, random_state=0, verbose=0, warm_start=False) |
There was a problem hiding this comment.
This is part of docstring, but take care with pep8
There was a problem hiding this comment.
This deprecation is also broken because not specifying a value will lead to a change in behavior between versions :-/
Do you mean because we're currently defaulting to min_impurity_split=1e-7 being applied together with min_impurity_decrease? Are we able to hope that this will only rarely affect results?
|
Yes because right now |
|
So we should have been doing a change of default before a deprecation.
Should we just go back and do that?? I think breaking everyone's tree
models without warning is highly unideal.
|
|
closing as we need to wait longer now |
Working on #11992.
This needs more work inside the tree code, though.