-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-6336] LBFGS should document what convergenceTol means #5033
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
Conversation
|
Test build #28625 has started for PR 5033 at commit
|
|
Test build #28625 timed out for PR 5033 at commit |
|
Test FAILed. |
docs/mllib-optimization.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If linking to Breeze, would be better to link straight to its LBFGS. However I don't know if we want to promise that implementation detail.
Should we also be updating the javadoc for LBFGS.scala and other similar places where this param is documented?
I'm not sure this text helps someone understand what the value is or how to set it -- what are adjusted values? You're right, it's fairly opaque beyond being something relative (rather than something that might need to change with the input). The thing to know is that lower values mean more iterations and the value must be nonnegative. So how about something more like:
Controls how much relative change is still allowed when L-BFGS is considered to converge. Must be nonnegative. Lower values are less tolerant and therefore generally cause more iterations to be run.
|
Test build #28648 has started for PR 5033 at commit
|
|
Test build #28648 timed out for PR 5033 at commit |
|
Test FAILed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: works better as "Lower convergence values are less tolerant and therefore ..." as you have elsewhere. (I can get to this on merge if you don't first.) LGTM otherwise. I think we can ignore the Jenkins timeout. I will verify the docs build before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for my unnatural expression. I modified to be more natural. Thank you.
|
Test build #28722 has started for PR 5033 at commit
|
|
Test build #28722 has finished for PR 5033 at commit
|
|
Test PASSed. |
|
Merged into master and branch-1.3. Thanks! |
LBFGS uses convergence tolerance. This value should be written in document as an argument. Author: lewuathe <[email protected]> Closes #5033 from Lewuathe/SPARK-6336 and squashes the following commits: e738b33 [lewuathe] Modify text to be more natural ac03c3a [lewuathe] Modify documentations 6ccb304 [lewuathe] [SPARK-6336] LBFGS should document what convergenceTol means (cherry picked from commit d9f3e01) Signed-off-by: Xiangrui Meng <[email protected]>
LBFGS uses convergence tolerance. This value should be written in document as an argument.