Skip to content

[MRG+2] Fix learning_rate default value in update_terminal_regions() #6463

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

Closed
wants to merge 1 commit into from

Conversation

movelikeriver
Copy link

Fix learning_rate default value in update_terminal_regions() in gradient_boosting.py

@agramfort
Copy link
Member

is this fixing a known issue? can we have a test? How did you spot the pb?

@movelikeriver
Copy link
Author

it's claimed as default value of 0.1 in comment, and also in caller function, so the default value of 0.1 is the expected behavior.
I don't any existing bug because of that at this moment, since it's only used in gradient_boosting.py, and already taken care by caller's default value.
It should be fixed to avoid potential bug in future.

@amueller
Copy link
Member

amueller commented Oct 8, 2016

I think this is good and a typo fix. @pprett?
LGTM

@amueller amueller changed the title Fix learning_rate default value in update_terminal_regions() [MRG + 1] Fix learning_rate default value in update_terminal_regions() Oct 8, 2016
@jnothman
Copy link
Member

So the alternative is to update the documentation to match the value, and hence not affect user code. If we update the value to match the documentation, we should at least note this clearly in What's New, as it's a breach of backwards compatibility.

@amueller
Copy link
Member

I don't thing this will affect any user code, unless then "manually" called update_terminal_regions. Which I guess is technically public API.

@raghavrv
Copy link
Member

This would atleast need a whatsnew entry... Otherwise LGTM...

@jnothman
Copy link
Member

Yes, please add a bug fix entry to whats_new.rst, then we can merge.

@jnothman jnothman changed the title [MRG + 1] Fix learning_rate default value in update_terminal_regions() [MRG+2] Fix learning_rate default value in update_terminal_regions() Dec 27, 2016
@rth
Copy link
Member

rth commented Jul 21, 2018

@glemaitre could you have a look at this? Apart for the missing what's new entry I think it might be good to merge with a +2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants