Fixing renmants of argument keyword in public API #2636
Conversation
| { | ||
| return new FastTreeBinaryClassificationTrainer.ObjectiveImpl( | ||
| TrainSet, | ||
| ConvertTargetsToBool(TrainSet.Targets), | ||
| Args.LearningRates, |
There was a problem hiding this comment.
Args [](start = 16, length = 4)
Why nor Options? #Closed
There was a problem hiding this comment.
I wanted to give a name to this property so that users would easily be able to distinguish to from the class Options .
In some cases it causes conflicts in our code itself , to have a property and class with same name
In reply to: 258278906 [](ancestors = 258278906)
| @@ -104,7 +104,7 @@ public sealed class Options : OptionsBase | |||
| internal MulticlassLogisticRegression(IHostEnvironment env, Options options) | |||
| : base(env, options, TrainerUtils.MakeU4ScalarColumn(options.LabelColumn)) | |||
| { | |||
| ShowTrainingStats = Args.ShowTrainingStats; | |||
| ShowTrainingStats = LbfgsTrainerOptions.ShowTrainingStats; | |||
There was a problem hiding this comment.
LbfgsTrainerOptions [](start = 32, length = 19)
Same here why not naming this Options? #Closed
There was a problem hiding this comment.
Mostly for clarity. Throughout the public API surface we have the class Options to represent a particular concept.
I wanted to name this property something different , so users can draw the distinction with the Options class
In some cases it causes conflicts in our code itself , to have a property and class with same name
In reply to: 258279561 [](ancestors = 258279561)
| @@ -73,7 +73,7 @@ public Options() | |||
| { | |||
| Host.CheckNonEmpty(featureColumn, nameof(featureColumn)); | |||
| Host.CheckNonEmpty(labelColumn, nameof(labelColumn)); | |||
| _loss = loss ?? Args.LossFunction.CreateComponent(env); | |||
| _loss = loss ?? SdcaTrainerOptions.LossFunction.CreateComponent(env); | |||
There was a problem hiding this comment.
SdcaTrainerOptions [](start = 28, length = 18)
Here as well? why not Options? #Closed
There was a problem hiding this comment.
Mostly for clarity, to help users distinguish between this property v/s class Options
In some cases it causes conflicts in our code itself , to have a property and class with same name
In reply to: 258279715 [](ancestors = 258279715)
| @@ -200,13 +200,13 @@ private void Run(IChannel ch) | |||
|
|
|||
| if (_model == null) | |||
| { | |||
| if (string.IsNullOrEmpty(Args.InputModelFile)) | |||
| if (string.IsNullOrEmpty(ImplOptions.InputModelFile)) | |||
There was a problem hiding this comment.
ImplOptions [](start = 41, length = 11)
If possible could you not use abbreviations? And here too, just curious why not Options? #Closed
There was a problem hiding this comment.
| @@ -20,23 +20,23 @@ public abstract class ArgumentsBase | |||
| } | |||
|
|
|||
| protected readonly IHost Host; | |||
| protected readonly TArgs Args; | |||
| protected readonly TOptions BaseSubsetSelectorOptions; | |||
There was a problem hiding this comment.
BaseSubsetSelectorOptions [](start = 36, length = 25)
Why not Options? #Closed
There was a problem hiding this comment.
Codecov Report
@@ Coverage Diff @@
## master #2636 +/- ##
=========================================
Coverage ? 71.53%
=========================================
Files ? 801
Lines ? 141840
Branches ? 16119
=========================================
Hits ? 101471
Misses ? 35919
Partials ? 4450
|
ghost
locked as resolved and limited conversation to collaborators
Fixes #2557