-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixing inconsistency in usage of LossFunction #2856
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
Changes from all commits
e3fe31e
1fefaa0
fcf85a2
22dbc78
ddd89fa
1405be3
5992842
bedd7f5
24c52e1
21ea513
400f552
56b1af5
1c204e3
c53ea0b
7051c24
4c5e033
5bd6e43
8673943
dd4b6f8
2ba494d
ffa170b
b4ec44f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1649,8 +1649,16 @@ public sealed class Options : BinaryOptionsBase | |
/// <value> | ||
/// If unspecified, <see cref="LogLoss"/> will be used. | ||
/// </value> | ||
[Argument(ArgumentType.Multiple, HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)] | ||
public ISupportSdcaClassificationLossFactory LossFunction = new LogLossFactory(); | ||
[Argument(ArgumentType.Multiple, Name = "LossFunction", HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)] | ||
internal ISupportSdcaClassificationLossFactory LossFunctionFactory = new LogLossFactory(); | ||
|
||
/// <summary> | ||
/// The custom <a href="tmpurl_loss">loss</a>. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What dose it link to? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/// </summary> | ||
/// <value> | ||
/// If unspecified, <see cref="LogLoss"/> will be used. | ||
/// </value> | ||
public ISupportSdcaClassificationLoss LossFunction { get; set; } | ||
} | ||
|
||
internal SdcaNonCalibratedBinaryTrainer(IHostEnvironment env, | ||
|
@@ -1666,7 +1674,7 @@ internal SdcaNonCalibratedBinaryTrainer(IHostEnvironment env, | |
} | ||
|
||
internal SdcaNonCalibratedBinaryTrainer(IHostEnvironment env, Options options) | ||
: base(env, options, options.LossFunction.CreateComponent(env)) | ||
: base(env, options, options.LossFunction ?? options.LossFunctionFactory.CreateComponent(env)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only constructor of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, user will see a trainer with In reply to: 262724703 [](ancestors = 262724703,262723959) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, as per documentation line 1659: In reply to: 262726971 [](ancestors = 262726971,262724703,262723959) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might prefer that the value be present (the idea of In reply to: 262728446 [](ancestors = 262728446,262726971,262724703,262723959) |
||
{ | ||
} | ||
|
||
|
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.
Were you intending to update this? #Resolved
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.
Also I noted elsewhere you noted what the replacement value would be, which struck me as positive. Will we do that here? (That said, that is merely documentation, which while important, I would not prioritize since the important thing is to get the shape of the API right, which you seem to have done here.)
In reply to: 263108655 [](ancestors = 263108655)
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.
Shahab is working on this, issue #2356
In reply to: 263109039 [](ancestors = 263109039,263108655)