Skip to content

[MRG+1] fix sign in mathematical formulation #9885

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 2 commits into from
Oct 20, 2017
Merged

[MRG+1] fix sign in mathematical formulation #9885

merged 2 commits into from
Oct 20, 2017

Conversation

Kyeongpil
Copy link

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

In the document (http://scikit-learn.org/stable/modules/ensemble.html#mathematical-formulation),
I found invalid sign in the formula.
Please refer the wikipedia following link:
https://en.wikipedia.org/wiki/Gradient_boosting#Algorithm

@jnothman
Copy link
Member

jnothman commented Oct 8, 2017 via email

@Kyeongpil
Copy link
Author

Kyeongpil commented Oct 9, 2017 •
edited
Loading

@jnothman yeah, I saw the code

At the _fit_stage function of BaseGradientBoosting in gradient_bossting.py,
new hypothesis h(decision tree) is trained by minimizing the loss between residual and its predicted value.

here the formulation of this loss can be expressed as I changed.

@TomDLT
Copy link
Member

TomDLT commented Oct 9, 2017 •
edited
Loading

The next formula with gradient descent is also wrong, isn't it?
F_m(x) = F_{m-1}(x) - \gamma_m \sum_{i=1}^{n} \nabla_F ...

@Kyeongpil
Copy link
Author

@TomDLT Oh right. i think the next formula should be also updated!

@Kyeongpil
Copy link
Author

Kyeongpil commented Oct 9, 2017 •
edited
Loading

@TomDLT I changed the sign in the mentioned formulation and commited. Thank you!

@Kyeongpil
Copy link
Author

It's my first pull request to scikit-learn and I don't know the merging process well.
Is there anything else I need to do for this pull?

TomDLT
TomDLT approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

LGTM

@TomDLT TomDLT changed the title fix sign in mathematical formulation [MRG+1] fix sign in mathematical formulation Oct 18, 2017
@lesteve
Copy link
Member

lesteve commented Oct 19, 2017 •
edited
Loading

Maybe @glemaitre you can have a quick look at this one since you are quite familiar with your work on the tree code recently?

this PR doc vs dev doc

@rudvlf0413 it would help if you added links to the code so it is easier for a reviewer to see that the code and the formula match. Look at this link if you are not familiar about how you can do that.

@glemaitre
Copy link
Member

glemaitre commented Oct 19, 2017 via email

@glemaitre
Copy link
Member

@lesteve after checking Hastie et al. and the internal implementation, LGTM.

@Kyeongpil
Copy link
Author

Kyeongpil commented Oct 20, 2017 •
edited
Loading

@lesteve I also checked the code where the formula is implemented
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/ensemble/gradient_boosting.py#L767
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/ensemble/gradient_boosting.py#L789
I think these codes are matched the first formula that I fixed.

@glemaitre Thank you for checking this! :D

@lesteve
Copy link
Member

lesteve commented Oct 20, 2017

OK merging, thanks a lot!

@lesteve lesteve merged commit e0a2997 into scikit-learn:master Oct 20, 2017
amueller pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 20, 2017
@amueller
[MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885
6a042a7 
)
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 21, 2017
@jnothman
[MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885
5f934e2 
)
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge tag '0.19.1' into releases
40884af 
* tag '0.19.1': (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge branch 'releases' into dfsg
0cad75b 
* releases: (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Oct 24, 2017
@yarikoptic
Merge branch 'dfsg' into debian
25061b8 
* dfsg: (117 commits)
  TST Improve SelectFromModel tests (scikit-learn#9733)
  Name in what's new
  [MRG+1] Raise error when SparseSeries is passed into classification metrics (scikit-learn#7373)
  Fix LogisticRegressionCV default solver value in docstring (scikit-learn#9962)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  [MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885)
  DOC fix a typo (scikit-learn#9892)
  [MRG+1] Ledoit-Wolf behavior explanation (scikit-learn#9500)
  [MRG+1] Fix typos in documentation (scikit-learn#9878)
  DOC: Use setattr(self, ...) instead of self.setattr(...) (scikit-learn#9866)
  DOC Removed a duplicate occurrence of a word in 'sklearn.neighbors.KNeighborsRegressor' docs (scikit-learn#9862)
  FIX docstring of negative_outlier_factor_ in LOF (scikit-learn#9809)
  [MRG+1] Fix scikit-learn#9743: Adding parameter information to docstring. (scikit-learn#9757)
  DOC: fix docstring of Imputer.fit (scikit-learn#9769)
  various minor spelling tweaks (scikit-learn#9783)
  MAINT comment on apparent inconsistency
  [MRG+1] DOC fix headers level in cross_validation.rst (scikit-learn#9679)
  Fix mailmap format (scikit-learn#9620)
  DOC Fix typos (scikit-learn#9577)
  Typo (scikit-learn#9571)
  ...
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@maskani-moh
[MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885
ae0377a 
)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
[MRG+1] DOC fix sign in GBRT mathematical formulation (scikit-learn#9885
c75df0d 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomDLT TomDLT TomDLT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kyeongpil @jnothman @TomDLT @lesteve @glemaitre