Skip to content

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

Closed
movelikeriver wants to merge 1 commit intoscikit-learn:masterfrom
movelikeriver:fix
Closed

[MRG+2] Fix learning_rate default value in update_terminal_regions()#6463
movelikeriver wants to merge 1 commit intoscikit-learn:masterfrom
movelikeriver:fix

Conversation

@movelikeriver
Copy link
Copy Markdown

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

@agramfort
Copy link
Copy Markdown
Member

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

@movelikeriver
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@jnothman
Copy link
Copy Markdown
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
Copy Markdown
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