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 n_estimators as a tunable parameter for XGBoost #307

Merged
merged 5 commits into from
Jan 21, 2020

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Jan 7, 2020

Added n_estimator into the list of possible tunable hyperparameters

Resolves #292

@@ -16,17 +16,20 @@ class XGBoostClassifier(Estimator):
"eta": Real(0, 1),
"max_depth": Integer(1, 20),
"min_child_weight": Real(1, 10),
"n_estimators": Integer(1, 100),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would > 100 trees be beneficial? Maybe we can pull up empirical evidence or heuristics using XGBoost parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs are pretty sparse on this. A quick search shows that the default is 100. Maybe we can bump this up to 1000?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can be more lax on ranges of hyper parameters if users run enough iterations. I think 1000 would be ok. @dsherry any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think until we can get experimental results to guide our thoughts, we should do what feels right. The current default is 100; I'd be surprised if using fewer estimators resulted in a performance boost. On the other hand, if we allow too many, it's possible that could affect memory consumption. For now, I'd say 1 - 1000 is fine.

If you're able to find any empirical evidence to back these numbers, that would be great.

Also, this reminds me that a good experiment to run would be to create a branch where we fix n_estimators at 1000, then run the perf tests. This will prove memory isn't an issue, at least in our perf test env.

@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #307 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   97.01%   97.05%   +0.03%     
==========================================
  Files          95       95              
  Lines        2950     2950              
==========================================
+ Hits         2862     2863       +1     
+ Misses         88       87       -1
Impacted Files Coverage Δ
evalml/pipelines/classification/xgboost.py 100% <ø> (ø) ⬆️
evalml/tests/pipeline_tests/test_xgboost.py 100% <100%> (ø) ⬆️
evalml/tests/component_tests/test_components.py 100% <100%> (ø) ⬆️
...nents/estimators/classifiers/xgboost_classifier.py 100% <100%> (ø) ⬆️
evalml/automl/auto_base.py 93.1% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56a33c...084d5b8. Read the comment docs.

@@ -22,8 +22,9 @@ class XGBoostPipeline(PipelineBase):
"eta": Real(0, 1),
"min_child_weight": Real(1, 10),
"max_depth": Integer(1, 20),
"n_estimators": Integer(1, 100),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christopherbunn I believe we have to update this range as well, right?

@@ -58,7 +58,7 @@ def test_describe_component():
linear_regressor = LinearRegressor()
assert lr_classifier.describe(return_dict=True) == {'name': 'Logistic Regression Classifier', 'parameters': {'C': 1.0, 'penalty': 'l2'}}
assert rf_classifier.describe(return_dict=True) == {'name': 'Random Forest Classifier', 'parameters': {'max_depth': 3, 'n_estimators': 10}}
assert xgb_classifier.describe(return_dict=True) == {'name': 'XGBoost Classifier', 'parameters': {'eta': 0.1, 'max_depth': 3, 'min_child_weight': 1}}
assert xgb_classifier.describe(return_dict=True) == {'name': 'XGBoost Classifier', 'parameters': {'eta': 0.1, 'max_depth': 3, 'min_child_weight': 1, 'n_estimators': 100}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a suggestion. Since we set the other XGBoostClassifier parameters to custom values in this test (a few lines up), could you also set n_estimators to a custom value which isn't the default of 100? This will provide coverage that changing the parameter via the pipeline constructor is in fact working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I see the test in test_xg_init below is doing something similar, and that technically we're already covered there. I'd argue my point here still stands though -- the more coverage the better! Plus the test_xg_init test is just checking the parameters field, but this test is checking the output of describe which is a bit different.)

expected_parameters = {'impute_strategy': 'median', 'percent_features': 1.0, 'threshold': -np.inf,
'eta': 0.2, 'max_depth': 5, 'min_child_weight': 3}
'eta': 0.2, 'max_depth': 5, 'min_child_weight': 3, 'n_estimators': 10}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@@ -31,7 +31,8 @@ def test_xg_multi(X_y_multi):
estimator = XGBClassifier(random_state=0,
eta=0.1,
max_depth=3,
min_child_weight=1)
min_child_weight=1,
n_estimators=10)
Copy link
Contributor

@dsherry dsherry Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this to 10? Is this just so the call to fit runs faster?

This could be a perfect place to use the mocking pattern I've discussed with @jeremyliweishih , in order to avoid actually fitting, to keep the unit tests fast. (That's tracked by #275 , no need to add mocking in this PR, just mentioning it for context/discussion)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I just chose a number on the lower end to make it run faster. Definitely open to changing this/not including this parameter at all if need be.

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Left some more comments

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 👏

@christopherbunn christopherbunn dismissed jeremyliweishih’s stale review January 11, 2020 01:01

Chose to set number of trees to 1000

@christopherbunn christopherbunn merged commit e988a93 into master Jan 21, 2020
@christopherbunn christopherbunn deleted the xgboost-nestimators branch January 21, 2020 15:39
@angela97lin angela97lin mentioned this pull request Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add n_estimators to hyperparameters for XGBoostClassifier
3 participants