-
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
Conversation
…ssFactory interfaces
nope this cant as we have this one that needs to stay public: public interface ISupportSdcaClassificationLoss : ISupportSdcaLoss, IClassificationLoss In reply to: 469863324 [](ancestors = 469863324) Refers to: src/Microsoft.ML.Data/Dirty/ILoss.cs:43 in 24c52e1. [](commit_id = 24c52e1, deletion_comment = False) |
nope this cant as we have this one that needs to stay public: public interface ISupportSdcaClassificationLoss : ISupportSdcaLoss, IClassificationLoss In reply to: 469863409 [](ancestors = 469863409) Refers to: src/Microsoft.ML.Data/Dirty/ILoss.cs:33 in 24c52e1. [](commit_id = 24c52e1, deletion_comment = False) |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
tmpurl_loss [](start = 36, length = 11)
What dose it link to? #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.
/// <value> | ||
/// If unspecified, <see cref="LogLoss"/> will be used. | ||
/// </value> | ||
public ISupportSdcaClassificationLoss LossFunction; |
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.
LossFunction [](start = 50, length = 12)
It doesn't look like we're using the same pattern Tom mentioned this morning. Please take a look at the following example for hiding EarlyStoppingRuleFactory
via adding a new EarlyStoppingRule
.
/// <summary>
/// Early stopping rule. (Validation set (/valid) is required).
/// </summary>
[BestFriend]
[Argument(ArgumentType.Multiple, HelpText = "Early stopping rule. (Validation set (/valid) is required.)", ShortName = "esr", NullName = "<Disable>")]
[TGUI(Label = "Early Stopping Rule", Description = "Early stopping rule. (Validation set (/valid) is required.)")]
internal IEarlyStoppingCriterionFactory EarlyStoppingRuleFactory;
private EarlyStoppingRuleBase _earlyStoppingRuleBase;
public EarlyStoppingRuleBase EarlyStoppingRule
{
get { return _earlyStoppingRuleBase; }
set
{
_earlyStoppingRuleBase = value;
EarlyStoppingRuleFactory = _earlyStoppingRuleBase.BuildFactory();
}
}
``` #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.
I dont see benefits of the changes you are proposing. What does _earlyStoppingRuleBase.BuildFactory(); do? How does it know what factory to build out if I set non default EarlyStoppingRule ?
In reply to: 262721736 [](ancestors = 262721736)
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.
One benefit is that the way Wei-Sheng proposes means that the object does not really enter any sort of inconsistent state, even for our internal code, thereby making the object easier to maintain. We could of course say, since the only thing touching this object at that level is this code, that we can avoid any flaws, but -- let's be honest with ourselves, even internal inconsistencies often show up as bugs externally. :) If you don't want to do it right now @ganik that is fine with me, but I would prefer in that case if we could make it a property so that we could adjust it later, if we believe it is necessary, in subsequent versions, without it being a breaking change. (So we'd change, here, public ISupportSdcaClassificationLoss LossFunction { get; set; }
so that it becomes an auto-implemented property.
In reply to: 262729182 [](ancestors = 262729182,262721736)
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.
Just going to re-activate for tracking. Like I say, you don't have to do it right now, but I'd rather it be a property in this case.
In reply to: 263112293 [](ancestors = 263112293,262729182,262721736)
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.
Sure I am fine with properties. For me the importance was that as third party dev I can supply my own LossFunction definition and will not be restricted to few pre-defined loss functions: LogLoss, HingeLoss etc. With Wei-Sheng pattern this becomes prohibitive.
In reply to: 263112433 [](ancestors = 263112433,263112293,262729182,262721736)
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.
Modified to properties
In reply to: 263631467 [](ancestors = 263631467,263112433,263112293,262729182,262721736)
@@ -1666,7 +1680,7 @@ public sealed class Options : BinaryOptionsBase | |||
} | |||
|
|||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The only constructor of Options
assigns new LogLoss()
to LossFunction
. Is it still possible to execute CreateComponent
? #ByDesign
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.
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.
Then, user will see a trainer with null
loss, but that trainer is minimizing LogLoss
. That's why I implemented a pattern mentioned by Tom.
In reply to: 262724703 [](ancestors = 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.
sure, as per documentation line 1659:
If unspecified, will be used.
In reply to: 262726971 [](ancestors = 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.
I might prefer that the value be present (the idea of null
acting like a particular specific instance is a bit suboptimal), but I don't insist on it I guess...
In reply to: 262728446 [](ancestors = 262728446,262726971,262724703,262723959)
/// </value> | ||
public ISupportSdcaClassificationLoss LossFunction; | ||
|
||
public Options() |
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.
Please add user-facing document. Thanks. #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.
[Argument(ArgumentType.Multiple, Name = "LossFunction", HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)] | ||
public ISupportSdcaClassificationLossFactory LossFunctionFactory = new LogLossFactory(); | ||
|
||
public ISupportSdcaClassificationLoss LossFunction; |
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.
Please add user-facing document. #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.
|
||
[Argument(ArgumentType.AtMostOnce, HelpText = "The calibrator kind to apply to the predictor. Specify null for no calibration", Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)] | ||
internal ICalibratorTrainerFactory Calibrator = new PlattCalibratorTrainerFactory(); | ||
|
||
[Argument(ArgumentType.AtMostOnce, HelpText = "The maximum number of examples to use when training the calibrator", Visibility = ArgumentAttribute.VisibilityType.EntryPointsOnly)] | ||
public int MaxCalibrationExamples = 1000000; | ||
|
||
public Options() |
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.
Please add user-facing document. #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.
internal ISupportSdcaRegressionLossFactory LossFunctionFactory = new SquaredLossFactory(); | ||
|
||
/// <summary> | ||
/// A custom <a href="tmpurl_loss">loss</a>. |
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.
tmpurl_loss [](start = 34, length = 11)
Not sure what this is.. #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.
see the line 41? same documentation as for the loss factory
In reply to: 262724768 [](ancestors = 262724768)
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.
Then please fix both. Maybe you can git blame to find the killer.
In reply to: 262725105 [](ancestors = 262725105,262724768)
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.
Codecov Report
@@ Coverage Diff @@
## master #2856 +/- ##
==========================================
+ Coverage 71.69% 71.81% +0.12%
==========================================
Files 809 812 +3
Lines 142427 142643 +216
Branches 16112 16095 -17
==========================================
+ Hits 102108 102437 +329
+ Misses 35890 35824 -66
+ Partials 4429 4382 -47
|
Note that the scope of this work is not to hide the loss functions -- as Gani points out that is impossible. THe scope of the work is to hide the empty-loss function factory interfaces, which exist merely to support the presence of this In reply to: 469876577 [](ancestors = 469876577,469863324) Refers to: src/Microsoft.ML.Data/Dirty/ILoss.cs:43 in 24c52e1. [](commit_id = 24c52e1, deletion_comment = False) |
internal ISupportClassificationLossFactory LossFunctionFactory1 = new HingeLoss.Options(); | ||
|
||
/// <summary> | ||
/// A custom <a href="tmpurl_loss">loss</a>. |
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.
tmpurl_loss [](start = 34, length = 11)
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.
[Argument(ArgumentType.Multiple, HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)] | ||
public ISupportClassificationLossFactory LossFunction = new HingeLoss.Options(); | ||
[Argument(ArgumentType.Multiple, Name = "LossFunction", HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)] | ||
internal ISupportClassificationLossFactory LossFunctionFactory1 = new HingeLoss.Options(); |
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.
LossFunctionFactory1 [](start = 55, length = 20)
Was the 1
here intentional? #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.
I guess it was... would it be possible to give it a more clear name? As far as I can tell the choice of 1
was just an arbitrary value so that it didn't conflict with the existing LossFunctionFactory
property, but ...
In reply to: 263109093 [](ancestors = 263109093)
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.
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.
Thank you @ganik !!
fixes #2174
fixes #2594