-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add needed param to warning for L2 in SDCA #597
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
Pull from upstream
Update to origin
Update to origin
@@ -209,7 +209,7 @@ internal virtual void Check(IHostEnvironment env) | |||
{ | |||
ch.Warning("The specified l2Const = {0} is too small. SDCA optimizes the dual objective function. " + | |||
"The dual formulation is only valid with a positive L2 regularization. Also, an l2Const less than {1} " + | |||
"could drastically slow down the convergence. So using l2Const = {1} instead.", L2Const); | |||
"could drastically slow down the convergence. So using l2Const = {1} instead.", L2Const, L2LowerBound); |
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.
l2Const [](start = 83, length = 7)
Hi @justinormont thanks for looking at this. Another problem with this message, replace these with nameof(L2Const)
, here, and everywhere in this message that it is used, in the format args. It might go easier if you used the $
string interpolation. #resolved
@dotnet-bot test OSX10.13 Debug |
@TomFinley : Perhaps there's a StyleCop (ref: #553) rule we can enable, which can catch future issues like this mismatched (param count) vs. (count of items in the |
Such a code analysis rule would have to be aware of In reply to: 408797756 [](ancestors = 408797756) |
If you wanted to improve these messages while you're at it so they don't contain this redundant information that would be nice. E.g., "``L2 constant must be non-negative" Refers to: src/Microsoft.ML.StandardLearners/Standard/LinearClassificationTrainer.cs:201 in dc42fee. [](commit_id = dc42fee, deletion_comment = False) |
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.
Thanks @justinormont . If you wanted to improve those other badly structured messages preceding your changes that would also help improve our overall quality.
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.
@TomFinley : I did a pass of word-smithing. The warning now reads as: |
* Fix warning for L2 in SDCA * String interpolation * Word-smithing the warning message
Closes #596