Skip to content

DOC add missing attributes in HistGradientBoostingRegressor #17677

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 24, 2020
Merged

DOC add missing attributes in HistGradientBoostingRegressor #17677

merged 12 commits into from
Jun 24, 2020

Conversation

simonamaggio
Copy link
Contributor

Fixes part of #14312 for HistGradientBoostingRegressor estimator.

Added bin_mapper_ , do_early_stopping_, loss_, n_features_, scorer_ to docstring in HistGradientBoostingRegressor.
HistGradientBoostingRegressor removed from the list of estimators in test_docstring_parameters.py.

@cmarmo cmarmo mentioned this pull request Jun 23, 2020
Merged
@cmarmo
Copy link
Contributor

cmarmo commented Jun 23, 2020

Hi @simonamaggio, thanks for your pull request.
Please have a look to #16283. HistGradientBoostingClassifier has similar structure and you will probably have similar reviews.
In particular about attributes that should remain private.

@simonamaggio
Copy link
Contributor Author

simonamaggio commented Jun 24, 2020 •
edited
Loading

Hi @simonamaggio, thanks for your pull request.
Please have a look to #16283. HistGradientBoostingClassifier has similar structure and you will probably have similar reviews.
In particular about attributes that should remain private.

Thanks @cmarmo. I had a look at the pointed comment and PR.
So two actions to do, but I'll wait for the reviews:

  1. removing trailing underscore
  2. make bin_mapper_, loss_, n_features_, scorer_ private.

@NicolasHug
Copy link
Member

@simonamaggio it looks like most of it was taken care of in #17678

the only thing left to do is to document the do_early_stopping attribute of the regressor. Please make sure to use the same description as in https://github.com/scikit-learn/scikit-learn/pull/17678/files#diff-3adddfacb3ffd10c1e81210ef2bea2abR1114

@cmarmo
Copy link
Contributor

cmarmo commented Jun 24, 2020

@simonamaggio it looks like most of it was taken care of in #17678

the only thing left to do is to document the do_early_stopping attribute of the regressor. Please make sure to use the same description as in https://github.com/scikit-learn/scikit-learn/pull/17678/files#diff-3adddfacb3ffd10c1e81210ef2bea2abR1114

I have to apologize to you both... I did a 'find'-'replace' without making a difference between estimators.... @simonamaggio if you want to include my changes and only merge one PR, feel free to do it.

@simonamaggio
Copy link
Contributor Author

I don't understand what to do here. Maybe @cmarmo can add in the other PR the documentation for do_early_stopping_ for the regressor too, and I will withdraw my PR.

NicolasHug
NicolasHug approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

LGTM when green

cmarmo
cmarmo approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @simonamaggio

@ogrisel
Copy link
Member

ogrisel commented Jun 24, 2020

I am fine with this but why not keep the scorer_ public? Other classes such as GridSearchCV and RandomizedSearchCV that accept a scoring constructor parameter also expose a public scorer_ attribute.

@ogrisel
Copy link
Member

ogrisel commented Jun 24, 2020

Actually we already discussed that earlier:

#16283 (comment)

ogrisel
ogrisel approved these changes Jun 24, 2020
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM.

@ogrisel
Copy link
Member

ogrisel commented Jun 24, 2020

About the conflict I am not sure how to resolve it: has GaussianProcessClassifier already been dealt with?

@ogrisel
Copy link
Member

ogrisel commented Jun 24, 2020

Indeed: in #17698.

@glemaitre glemaitre self-assigned this Jun 24, 2020
@glemaitre glemaitre merged commit a109dff into scikit-learn:master Jun 24, 2020
@glemaitre
Copy link
Member

Merging after updating with master

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
@simonamaggio @ogrisel @glemaitre
DOC add missing attributes in HistGradientBoostingRegressor (scikit-l…
f3cd83b 
…earn#17677)

Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@simonamaggio @ogrisel
@glemaitre @jayzed82
DOC add missing attributes in HistGradientBoostingRegressor (scikit-l…
422f628 
…earn#17677)

Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@NicolasHug NicolasHug NicolasHug approved these changes

@cmarmo cmarmo cmarmo approved these changes

Assignees

@glemaitre glemaitre

Labels
module:ensemble Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@simonamaggio @cmarmo @NicolasHug @ogrisel @glemaitre