Skip to content
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

Updating LightGBM Arguments #2948

Merged
merged 17 commits into from Mar 19, 2019

Conversation

Projects
None yet
4 participants
@singlis
Copy link
Member

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.

  • Options code that used to live in LightGbmArguments has moved to LightGbmBaseTrainer. Each trainer now has their own respective options class.
  • The booster options have changed a bit to hide the IBoosterFactory interface #2559

Scott Inglis added some commits Mar 7, 2019

@singlis singlis requested review from codemzs , Ivanidzo4ka , wschin , TomFinley , ganik and sfilipi Mar 13, 2019

@singlis singlis self-assigned this Mar 13, 2019

@@ -8,7 +8,7 @@ resources:
image: microsoft/dotnet-buildtools-prereqs:centos-7-b46d863-20180719033416

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

This comment has been minimized.

@singlis

singlis Mar 13, 2019

Author Member

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.");

This comment has been minimized.

@singlis

singlis Mar 13, 2019

Author Member

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

This comment has been minimized.

@singlis

singlis Mar 13, 2019

Author Member

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

This comment has been minimized.

@singlis

singlis Mar 13, 2019

Author Member

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>()

This comment has been minimized.

@singlis

singlis Mar 13, 2019

Author Member

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

@@ -53,10 +53,17 @@ Trainers.FieldAwareFactorizationMachineBinaryClassifier Train a field-aware fact
Trainers.GeneralizedAdditiveModelBinaryClassifier Trains 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.Gam TrainBinary Microsoft.ML.Trainers.FastTree.GamBinaryClassificationTrainer+Options Microsoft.ML.EntryPoints.CommonOutputs+BinaryClassificationOutput
Trainers.GeneralizedAdditiveModelRegressor Trains 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.Gam TrainRegression Microsoft.ML.Trainers.FastTree.GamRegressionTrainer+Options Microsoft.ML.EntryPoints.CommonOutputs+RegressionOutput
Trainers.KMeansPlusPlusClusterer K-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.KMeansTrainer TrainKMeans Microsoft.ML.Trainers.KMeansTrainer+Options Microsoft.ML.EntryPoints.CommonOutputs+ClusteringOutput
<<<<<<< HEAD

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 13, 2019

Member

merge conflict
Just regenerate it locally and override #Closed

@@ -225,7 +225,8 @@ private string GetBuildPrefix()
#endif
}

[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]

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 13, 2019

Member

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";

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 13, 2019

Member

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

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

This comment has been minimized.

@singlis

singlis Mar 15, 2019

Author Member

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


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

ch.CheckValue(groups, nameof(groups));
// Add default metric.
if (!Options.ContainsKey("metric"))
Options["metric"] = "ndcg";
if (!GbmOptions.ContainsKey("metric"))

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 13, 2019

Member

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.LightGbmBinaryClassifier Train a LightGBM binary classification model. Microsoft.ML.Trainers.LightGbm.LightGbm TrainBinary Microsoft.ML.Trainers.LightGbm.Options Microsoft.ML.EntryPoints.CommonOutputs+BinaryClassificationOutput
Trainers.LightGbmClassifier Train a LightGBM multi class model. Microsoft.ML.Trainers.LightGbm.LightGbm TrainMulticlass Microsoft.ML.Trainers.LightGbm.Options Microsoft.ML.EntryPoints.CommonOutputs+MulticlassClassificationOutput
Trainers.LightGbmRanker Train a LightGBM ranking model. Microsoft.ML.Trainers.LightGbm.LightGbm TrainRanking Microsoft.ML.Trainers.LightGbm.Options Microsoft.ML.EntryPoints.CommonOutputs+RankingOutput
Trainers.LightGbmRegressor LightGBM Regression Microsoft.ML.Trainers.LightGbm.LightGbm TrainRegression Microsoft.ML.Trainers.LightGbm.Options Microsoft.ML.EntryPoints.CommonOutputs+RegressionOutput
>>>>>>> origin/master

This comment has been minimized.

@wschin

wschin Mar 13, 2019

Member

Who are you? #Resolved

Scott Inglis
/// 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>

This comment has been minimized.

@shmoradims

shmoradims Mar 14, 2019

Contributor

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

This comment has been minimized.

@singlis

singlis Mar 14, 2019

Author Member

thanks Shahab - I captured these.


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

}

public interface IBoosterParameter
internal interface IBoosterParameterFactory : IComponentFactory<BoosterParameterBase>
{

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

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

This comment has been minimized.

@singlis

singlis Mar 16, 2019

Author Member

I can run the command line.


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


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

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

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

This comment has been minimized.

@singlis

singlis Mar 15, 2019

Author Member

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);
}

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

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

This comment has been minimized.

@singlis

singlis Mar 16, 2019

Author Member

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)

This comment has been minimized.

@singlis

singlis Mar 16, 2019

Author Member

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


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

@@ -84,6 +89,42 @@ public sealed class LightGbmRegressionTrainer : LightGbmTrainerBase<float, Regre

private protected override PredictionKind PredictionKind => PredictionKind.Regression;

public sealed class Options : OptionsBase
{
public enum EvaluateMetricType

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

EvaluateMetricType [](start = 24, length = 18)

I would also add MeanSquaredError (mse) #Closed

None,
Default,
Map,
Ndcg

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

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

NormalizedDiscountedCumulativeGain or Lambdarank #Closed

{
None,
Default,
Map,

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

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";

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

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

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

None,
Default,
Merror,
Mlogloss,

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

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

LogLoss #Closed

{
None,
Default,
Merror,

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Member

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

Error #Closed

@codecov

This comment has been minimized.

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
@@ -35,10 +35,10 @@ Virtual memory usage(MB): %Number%
[1] 'Loading data for LightGBM' started.
[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

This comment has been minimized.

@singlis

singlis Mar 16, 2019

Author Member

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

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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

Scott Inglis added some commits Mar 18, 2019

Scott Inglis
Scott Inglis
Scott Inglis
[TlcModule.SweepableDiscreteParam("RegAlpha", new object[] { 0f, 0.5f, 1f })]
public double L1Regularization = 0;

BoosterParameterBase IComponentFactory<BoosterParameterBase>.CreateComponent(IHostEnvironment env)
{

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

{ [](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.");

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

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

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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)

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

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)

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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)

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

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

Constuctor of DartBooster is internal #Resolved

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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)

{
}

private protected override CalibratedModelParametersBase<LightGbmBinaryModelParameters, PlattCalibrator> CreatePredictor()
{
Host.Check(TrainedEnsemble != null, "The predictor cannot be created before training is complete");
var innerArgs = LightGbmInterfaceUtils.JoinParameters(Options);
var innerArgs = LightGbmInterfaceUtils.JoinParameters(base.GbmOptions);

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

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

Does base necessary? #Resolved

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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


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

@Ivanidzo4ka
Copy link
Member

Ivanidzo4ka left a comment

:shipit:

@@ -82,7 +85,7 @@ private static IPredictorProducing<float> Create(IHostEnvironment env, ModelLoad
/// The <see cref="IEstimator{TTransformer}"/> for training a boosted decision tree binary classification model using LightGBM.
/// </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,

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

missing space #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.");
Contracts.CheckUserArg(options.WeightOfPositiveExamples > 0, nameof(Options.WeightOfPositiveExamples), "must be >= 0.");

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

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

#Resolved

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

nice catch


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

@@ -144,10 +226,7 @@ private protected override void CheckDataValid(IChannel ch, RoleMappedData data)

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

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

One-line function uses => #Resolved

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

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

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

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

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,

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

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

WeightColumnName #Resolved

This comment has been minimized.

@singlis

singlis Mar 18, 2019

Author Member

I think rename should happen in a different PR.


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

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 18, 2019

Member

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")]

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

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

It is array now. #Resolved

@@ -225,6 +226,27 @@ public static string JoinParameters(Dictionary<string, object> parameters)
return string.Join(" ", res);
}

public static string GetOptionName(string name)
{

This comment has been minimized.

@wschin

wschin Mar 18, 2019

Member

Doc string please. #Resolved

@wschin

wschin approved these changes Mar 18, 2019

Copy link
Member

wschin left a comment

I only left some minor comments. Thank you!

Scott Inglis added some commits Mar 18, 2019

@singlis singlis merged commit aea88dc into dotnet:master Mar 19, 2019

3 checks passed

MachineLearning-CI #20190319.12 succeeded
Details
MachineLearning-CodeCoverage #20190319.12 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.