8000 [MRG+1] Corrected sign error in QuantileLossFunction by AlexisMignon · Pull Request #6429 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG+1] Corrected sign error in QuantileLossFunction #6429

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 6 commits into from
Dec 22, 2016
Merged

[MRG+1] Corrected sign error in QuantileLossFunction #6429

merged 6 commits into from
Dec 22, 2016

Conversation

AlexisMignon
Copy link
Contributor

There was an error sign for the computation of the QuantileLossFunction for negative errors.

from __future__ import print_function
from sklearn.ensemble.gradient_boosting import QuantileLossFunction
import numpy as np

# For positive errors only
x = np.linspace(0, 1, 101)
print("positive errors:", QuantileLossFunction(1, 0.5)(x, np.zeros(len(x))))

# For negative errors only
x = np.linspace(-1, 0, 101)
print("negative errors:", QuantileLossFunction(1, 0.5)(x, np.zeros(len(x))))

giving:

positive errors: 0.25
negative errors: -0.25

While it should be positive all the time.

@amueller
Copy link
Member
amueller commented Oct 8, 2016

I'm pretty sure this is right in the context where this is called, but I don't remember exactly. @pprett is probably not around? @glouppe might know?

@ahyoussef ahyoussef mentioned this pull request Dec 19, 2016
Closed
@jnothman
Copy link
Member

@AlexisMignon if you're still around, please add a non-regression test, along the lines of #8087, and a what's new entry, might be all that's needed to get this merged.

@jnothman
Copy link
Member

If @AlexisMignon doesn't handle this in a few days, you are welcome to complete the work, @ahyoussef

@agramfort
< 8000 summary data-view-component="true" class="timeline-comment-action Link--secondary Button--link Button--medium Button"> Copy link
Member

LGTM

please update what's new bug section

@jnothman
Copy link
Member

(The travis failure appears unrelated. May be fixed by #8097)

@AlexisMignon AlexisMignon changed the title Corrected sign error in QuantileLossFunction [MRG] Corrected sign error in QuantileLossFunction Dec 21, 2016
@agramfort
Copy link
Member

LGTM

@agramfort agramfort changed the title [MRG] Corrected sign error in QuantileLossFunction [MRG+1] Corrected sign error in QuantileLossFunction Dec 21, 2016
lesteve
lesteve reviewed Dec 21, 2016
View reviewed changes
doc/whats_new.rst
- Fix a bug where
:class:`sklearn.ensemble.gradient_boosting.QuantileLossFunction` computed
negative errors for negative values of ``ytrue - ypred`` leading to
wrong values when calling ``__call__``.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in a "Bug Fixes" rather than "Enhancements" section. Add a "Bug Fixes" section if there isn't one already.

Also add a link to the PR (with the issue directive) and a link to your profile with the user directive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well most probably some git oddities since it is in the bug section in my version of the file. Resyncing with master does not help.

@agramfort agramfort merged commit b685494 into scikit-learn:master Dec 22, 2016
@agramfort
Copy link
Member

thx @AlexisMignon

@amueller
Copy link
Member
amueller commented Jan 3, 2017

does this change any behavior?

@AlexisMignon
Copy link
Contributor Author
AlexisMignon commented Jan 5, 2017 •
edited
Loading

@amueller I don't think it does since internally only the gradient is used and their was no sign problem in the gradient formula. Needs some confirmation though.

@ahyoussef
Copy link

@amueller Same here: the crucial thing is the gradient and it was already correctly implemented. But it would be great if someone who knows all the details of the sklearn API could confirm.

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@AlexisMignon @sergeyf
[MRG+1] Corrected sign error in QuantileLossFunction (scikit-learn#6429)
b6c2f80 
* Corrected sign error in QuantileLossFunction
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Closed
@pprett pprett mentioned this pull request May 17, 2017
Closed
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
@AlexisMignon @Sundrique
[MRG+1] Corrected sign error in QuantileLossFunction (scikit-learn#6429)
4a4dfac 
* Corrected sign error in QuantileLossFunction
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@AlexisMignon @NelleV
[MRG+1] Corrected sign error in QuantileLossFunction (scikit-learn#6429)
285364b 
* Corrected sign error in QuantileLossFunction
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@AlexisMignon @paulha
[MRG+1] Corrected sign error in QuantileLossFunction (scikit-learn#6429)
8316c44 
* Corrected sign error in QuantileLossFunction
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@AlexisMignon @maskani-moh
[MRG+1] Corrected sign error in QuantileLossFunction (scikit-learn#6429)
7545363 
* Corrected sign error in QuantileLossFunction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesteve lesteve lesteve left review comments

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

Successfully merging this pull request may close these issues.
6 participants
@AlexisMignon @amueller @jnothman @agramfort @ahyoussef @lesteve
0