Skip to content

Conversation

@singlis
Copy link
Member

@singlis singlis commented Mar 13, 2019

This PR moves the LightGBM options from an all-in-one class to individual options for the binary, multiclass, regression and ranker trainers.


- container: UbuntuContainer
image: microsoft/dotnet-buildtools-prereqs:ubuntu-16.04-10fcdcf-20190208200917
image: microsoft/dotnet-buildtools-prereqs:ubuntu-16.04-mlnet-207e097-20190312152303
Copy link
Member Author

@singlis singlis Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore this change... #Resolved

internal LightGbmBinaryClassificationTrainer(IHostEnvironment env, Options options)
: base(env, LoadNameValue, options, TrainerUtils.MakeBoolScalarLabel(options.LabelColumnName))
{
Contracts.CheckUserArg(options.Sigmoid > 0, nameof(Options.Sigmoid), "must be > 0.");
Copy link
Member Author

@singlis singlis Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be on Multiclass and Ranking. #Resolved

// Contains the passed in options when the API is called
private protected readonly TOptions LightGbmTrainerOptions;

// Contains the GBMOptions
Copy link
Member Author

@singlis singlis Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GBMOptions [](start = 24, length = 10)

Ill move the argument below for this, I had rename to GbmOptions as it was conflicting with the Options class in the derived classes. #Resolved

LightGbmTrainerOptions.FeatureColumnName = featureColumnName;
LightGbmTrainerOptions.ExampleWeightColumnName = exampleWeightColumnName;
LightGbmTrainerOptions.RowGroupColumnName = rowGroupColumnName;
LightGbmTrainerOptions = new TOptions
Copy link
Member Author

@singlis singlis Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LightGbmTrainerOptions [](start = 12, length = 22)

will resolve this call the constructor below (via Options). #Resolved

// Static override name map that maps friendly names to lightGBM arguments.
// If an argument is not here, then its name is identicaltto a lightGBM argument
// and does not require a mapping, for example, Subsample.
private static Dictionary<string, string> _nameMapping = new Dictionary<string, string>()
Copy link
Member Author

@singlis singlis Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont like having a dictionary for this, even worse is having the dictionary defined here in this interface. If someone were to add new options to be handled in lightgbm, its not clear that they need to potentially update this as well in order for the correct lightgbm argument to get mapped. I think a better "option" (har har no pun intended) would be to have the Option classes build this dictionary, but I haven't worked on this yet and this at least gets things semi-working.
#Resolved

Trainers.GeneralizedAdditiveModelBinaryClassifierTrains a gradient boosted stump per feature, on all features simultaneously, to fit target values using least-squares. It mantains no interactions between features.Microsoft.ML.Trainers.FastTree.GamTrainBinaryMicrosoft.ML.Trainers.FastTree.GamBinaryClassificationTrainer+OptionsMicrosoft.ML.EntryPoints.CommonOutputs+BinaryClassificationOutput
Trainers.GeneralizedAdditiveModelRegressorTrains a gradient boosted stump per feature, on all features simultaneously, to fit target values using least-squares. It mantains no interactions between features.Microsoft.ML.Trainers.FastTree.GamTrainRegressionMicrosoft.ML.Trainers.FastTree.GamRegressionTrainer+OptionsMicrosoft.ML.EntryPoints.CommonOutputs+RegressionOutput
Trainers.KMeansPlusPlusClustererK-means is a popular clustering algorithm. With K-means, the data is clustered into a specified number of clusters in order to minimize the within-cluster sum of squares. K-means++ improves upon K-means by using a better method for choosing the initial cluster centers.Microsoft.ML.Trainers.KMeansTrainerTrainKMeansMicrosoft.ML.Trainers.KMeansTrainer+OptionsMicrosoft.ML.EntryPoints.CommonOutputs+ClusteringOutput
<<<<<<< HEAD
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge conflict
Just regenerate it locally and override #Closed


[Fact(Skip = "Execute this test if you want to regenerate the core_manifest and core_ep_list files")]
//[Fact(Skip = "Execute this test if you want to regenerate the core_manifest and core_ep_list files")]
[Fact]
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fact [](start = 9, length = 4)

don't forget to put it back. #Resolved

if (!Options.ContainsKey("metric"))
Options["metric"] = "multi_error";
if (!GbmOptions.ContainsKey("metric"))
GbmOptions["metric"] = "multi_error";
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"multi_error" [](start = 39, length = 13)

we don't use metric enum at all? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each trainer has its own respective metric enum and default metric type being assigned on initialization.


In reply to: 265372586 [](ancestors = 265372586)

// Add default metric.
if (!Options.ContainsKey("metric"))
Options["metric"] = "ndcg";
if (!GbmOptions.ContainsKey("metric"))
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!GbmOptions.ContainsKey("metric")) [](start = 12, length = 38)

we don't have base metrics anymore, so we should provide enum in options and set it value here. #Closed

Trainers.LightGbmClassifierTrain a LightGBM multi class model.Microsoft.ML.Trainers.LightGbm.LightGbmTrainMulticlassMicrosoft.ML.Trainers.LightGbm.OptionsMicrosoft.ML.EntryPoints.CommonOutputs+MulticlassClassificationOutput
Trainers.LightGbmRankerTrain a LightGBM ranking model.Microsoft.ML.Trainers.LightGbm.LightGbmTrainRankingMicrosoft.ML.Trainers.LightGbm.OptionsMicrosoft.ML.EntryPoints.CommonOutputs+RankingOutput
Trainers.LightGbmRegressorLightGBM RegressionMicrosoft.ML.Trainers.LightGbm.LightGbmTrainRegressionMicrosoft.ML.Trainers.LightGbm.OptionsMicrosoft.ML.EntryPoints.CommonOutputs+RegressionOutput
>>>>>>> origin/master
Copy link
Member

@wschin wschin Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who are you? #Resolved

/// If the tree partition step results in a leaf node with the sum of instance weight less than <see cref="MinimumChildWeight"/>,
/// the building process will give up further partitioning. In linear regression mode, this simply corresponds to minimum number
/// of instances needed to be in each node. The larger, the more conservative the algorithm will be.
/// </value>
Copy link

@shmoradims shmoradims Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's happening to all these xml documentations? do you need to merge with master? Please make sure to move the xml docs when moving around the options. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks Shahab - I captured these.


In reply to: 265696202 [](ancestors = 265696202)


public interface IBoosterParameter
internal interface IBoosterParameterFactory : IComponentFactory<BoosterParameterBase>
{
Copy link
Member

@wschin wschin Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a best friend? I don't know if command line framework can call it now. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can run the command line.


In reply to: 265815086 [](ancestors = 265815086)

res[GetOptionName(field.Name)] = field.GetValue(BoosterParameterOptions);
}
return BuildOptions();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your problem. C# doesn't work well here, so we must have two (conceptually) identical implementations..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for mentioning this, I will keep this active for other reviewers.


In reply to: 265815338 [](ancestors = 265815338)

base.UpdateParameters(res);
res["boosting_type"] = Name;
res[LightGbmInterfaceUtils.GetOptionName(field.Name)] = field.GetValue(BoosterOptions);
}
Copy link
Member

@wschin wschin Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if key LightGbmInterfaceUtils.GetOptionName(field.Name) exists as this function is called Update? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So LightGbmInterfaceUtils.GetOptionName does not have a dictionary that it works from -- it just converts the field name to lower case plus some underscoring. So there should be no need to check for a key. There could already be a key in the res dictionary - but thats ok as we are overriding it and like you said, this is an Update function.


In reply to: 265815549 [](ancestors = 265815549)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill mark this as pending as I want to make sure I answered your question.


In reply to: 266178686 [](ancestors = 266178686,265815549)

None,
Default,
Map,
Ndcg
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ndcg [](start = 16, length = 4)

NormalizedDiscountedCumulativeGain or Lambdarank #Closed

{
None,
Default,
Map,
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map [](start = 16, length = 3)

MeanAveragePrecision #Closed

/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Comma seperated list of gains associated to each relevance label.", ShortName = "gains")]
[TGUI(Label = "Ranking Label Gain")]
public string CustomGains = "0,3,7,15,31,63,127,255,511,1023,2047,4095";
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string CustomGains [](start = 19, length = 18)

public int[] CustomGains = new int[] { 0, 3, 7, 15, 31,63,... }; #Closed

None,
Default,
Merror,
Mlogloss,
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mlogloss [](start = 16, length = 8)

LogLoss #Closed

{
None,
Default,
Merror,
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merror [](start = 16, length = 6)

Error #Closed

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@0831865). Click here to learn what that means.
The diff coverage is 86.96%.

@@ Coverage Diff @@ ## master #2948 +/- ## ========================================= Coverage ? 72.35% ========================================= Files ? 803 Lines ? 143388 Branches ? 16154 ========================================= Hits ? 103750 Misses ? 35214 Partials ? 4424
Flag Coverage Δ
#Debug 72.35% <86.96%> (?)
#production 68.07% <86.34%> (?)
#test 88.52% <100%> (?)
Impacted Files Coverage Δ
...ML.LightGbm.StaticPipe/LightGbmStaticExtensions.cs 45.45% <0%> (ø)
...cenariosWithDirectInstantiation/TensorflowTests.cs 90.81% <100%> (ø)
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs 63.8% <100%> (ø)
.../Microsoft.ML.LightGbm/WrappedLightGbmInterface.cs 83.72% <100%> (ø)
src/Microsoft.ML.LightGbm/LightGbmBinaryTrainer.cs 87.71% <100%> (ø)
src/Microsoft.ML.LightGbm/LightGbmCatalog.cs 100% <100%> (ø)
...Microsoft.ML.LightGbm/LightGbmMulticlassTrainer.cs 89.57% <100%> (ø)
test/Microsoft.ML.TestFramework/Learners.cs 90.74% <100%> (ø)
...osoft.ML.Tests/TrainerEstimators/TreeEstimators.cs 98.52% <100%> (ø)
src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs 62.44% <75.58%> (ø)
... and 3 more
[1] 'Loading data for LightGBM' finished in %Time%.
[2] 'Training with LightGBM' started.
[2] (%Time%) Iteration: 50 Training-l2: 37.107605006517
[2] (%Time%) Iteration: 50 Training-rmse: 6.09160118577349
Copy link
Member Author

@singlis singlis Mar 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rmse [](start = 36, length = 4)

I am investigating the delta in these numbers. I believe it has to do with the metric type we were previously passing vs what is passed now. Previously we were setting the metric type to l2 which is incorrect for rmse. Now we are passing l2_root.
#Resolved

Copy link
Member Author

@singlis singlis Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this is due to changing the code to use l2_root(aka rmse), so this change is expected.


In reply to: 266183353 [](ancestors = 266183353)

@singlis singlis changed the title WIP: Updating LightGBM Arguments Updating LightGBM Arguments Mar 18, 2019
public double L1Regularization = 0;

BoosterParameterBase IComponentFactory<BoosterParameterBase>.CreateComponent(IHostEnvironment env)
{
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ [](start = 12, length = 1)

nit: you can just BoosterParameterBase IComponentFactory<BoosterParameterBase>.CreateComponent(IHostEnvironment env) => BuildOptions(); #Resolved

Contracts.CheckUserArg(options.SubsampleFraction > 0 && options.SubsampleFraction <= 1, nameof(Options.SubsampleFraction), "must be in (0,1].");
Contracts.CheckUserArg(options.FeatureFraction > 0 && options.FeatureFraction <= 1, nameof(Options.FeatureFraction), "must be in (0,1].");
Contracts.CheckUserArg(options.L2Regularization >= 0, nameof(Options.L2Regularization), "must be >= 0.");
Contracts.CheckUserArg(options.L1Regularization >= 0, nameof(Options.L1Regularization), "must be >= 0.");
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need this validation across all boosters? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - these are all in the BoosterParameterBase -- so GradientBooster, DartBooster and GossBooster all share these arguments. Therefore I added these restrictions. These restrictions come from the lightgbm documentation.


In reply to: 266665035 [](ancestors = 266665035)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you run validation only in GradientBooster DartBooster doesn't check this options as far as I can see.


In reply to: 266672503 [](ancestors = 266672503,266665035)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right - I was thinking DartBooster derives from Gradient, but it doesnt (*it used to :))


In reply to: 266673967 [](ancestors = 266673967,266672503,266665035)

/// </value>
[Argument(ArgumentType.AtMostOnce, HelpText = "Printing running messages.")]
public bool Silent = true;
public GradientBooster(Options options)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GradientBooster [](start = 15, length = 15)

Constuctor of DartBooster is internal #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that is actually intentional since the user doesnt construct a DartBooster, they construct a DartBooster.Options. However, you do point out that GradientBooster is public -- so I made that internal.


In reply to: 266665204 [](ancestors = 266665204)

{
Host.Check(TrainedEnsemble != null, "The predictor cannot be created before training is complete");
var innerArgs = LightGbmInterfaceUtils.JoinParameters(Options);
var innerArgs = LightGbmInterfaceUtils.JoinParameters(base.GbmOptions);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base [](start = 66, length = 4)

Does base necessary? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no that comes from the VS renaming....I will remove.


In reply to: 266665665 [](ancestors = 266665665)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

/// </summary>
/// <include file='doc.xml' path='doc/members/member[@name="LightGBM_remarks"]/*' />
public sealed class LightGbmBinaryClassificationTrainer : LightGbmTrainerBase<float,
public sealed class LightGbmBinaryClassificationTrainer : LightGbmTrainerBase<LightGbmBinaryClassificationTrainer.Options,float,
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space #Resolved

: base(env, LoadNameValue, options, TrainerUtils.MakeBoolScalarLabel(options.LabelColumnName))
{
Contracts.CheckUserArg(options.Sigmoid > 0, nameof(Options.Sigmoid), "must be > 0.");
Contracts.CheckUserArg(options.WeightOfPositiveExamples > 0, nameof(Options.WeightOfPositiveExamples), "must be >= 0.");
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

= [](start = 124, length = 2)

#Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch


In reply to: 266671856 [](ancestors = 266671856)

}

private protected override void CheckAndUpdateParametersBeforeTraining(IChannel ch, RoleMappedData data, float[] labels, int[] groups)
{
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One-line function uses => #Resolved

NameMapping.Add(nameof(EvaluateMetricType.Error), "multi_error");
NameMapping.Add(nameof(EvaluateMetricType.LogLoss), "multi_logloss");
}

Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not work with new Options(). #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is static, it will be created before our main function is called.


In reply to: 266672901 [](ancestors = 266672901)

{
LabelColumnName = labelColumnName,
FeatureColumnName = featureColumnName,
ExampleWeightColumnName = exampleWeightColumnName,
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExampleWeightColumnName [](start = 20, length = 23)

WeightColumnName #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rename should happen in a different PR.


In reply to: 266673053 [](ancestors = 266673053)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rename should happen in a different PR.
+1


In reply to: 266674022 [](ancestors = 266674022,266673053)

/// <summary>
/// Comma-separated list of gains associated with each relevance label.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Comma seperated list of gains associated to each relevance label.", ShortName = "gains")]
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma seperated list [](start = 59, length = 20)

It is array now. #Resolved

}

public static string GetOptionName(string name)
{
Copy link
Member

@wschin wschin Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc string please. #Resolved

Copy link
Member

@wschin wschin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only left some minor comments. Thank you!

@singlis singlis merged commit aea88dc into dotnet:master Mar 19, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

4 participants