Adding the extension methods for FastTree classification and regression #1009

sfilipi · 2018-09-24T23:04:14Z

OnGoing work to address #754.

Added the extensions to the Binary and Regression context for the respective FastTree trainers.
Ranking coming on a separate PR, since it needs a bit more work with the context and elevator changes.

sfilipi · 2018-09-24T23:06:25Z

src/Microsoft.ML.FastTree/FastTreeStatic.cs

+            Contracts.CheckParam(learningRates > 0, nameof(learningRates), "Must be positive");
+            Contracts.CheckValueOrNull(advancedSettings);
+            Contracts.CheckValueOrNull(onFit);
+


don't duplicate #Resolved

sfilipi · 2018-09-24T23:06:54Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

+                TrainerUtils.CheckArgsAndAdvancedSettingMismatch(numTrees, snapshot.NumTrees, Args.NumTrees, nameof(numTrees));
+                TrainerUtils.CheckArgsAndAdvancedSettingMismatch(minDocumentsInLeafs, snapshot.MinDocumentsInLeafs, Args.MinDocumentsInLeafs, nameof(minDocumentsInLeafs));
+                TrainerUtils.CheckArgsAndAdvancedSettingMismatch(learningRates, snapshot.LearningRates, Args.LearningRates, nameof(learningRates));
+            }


move in FastTreeUtils and re-use. #Resolved

Ivanidzo4ka · 2018-09-24T23:17:32Z

src/Microsoft.ML.Data/Training/TrainerUtils.cs

+            // if, after applying the advancedArgs delegate, the args are different that the default value
+            // and are also different than the value supplied directly to the xtension method, warn the user.
+            if (!setting.Equals(defaultVal) && !setting.Equals(methodParam))
+                Console.WriteLine($"The value supplied to advanced settings , is different than the value supplied directly. Using value {setting} for {argName}");


Console.WriteLine [](start = 15, length = 18)

I don't like this. Can we accept Host or Channel as first object and use it? #Closed

for sure; put it to look for a Warn method, or pass the channel, and forgot to replace.

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

Ivanidzo4ka · 2018-09-24T23:18:18Z

src/Microsoft.ML.Data/Training/TrainerUtils.cs

+        public static void CheckArgumesDefaultColNames(string defaultColName, string argValue)
+        {
+            if (argValue != defaultColName)
+                throw Contracts.Except($"Don't supply a value for the {defaultColName} column in the arguments, as it will be ignored. Specify them in the loader, or constructor instead instead.");


Everywhere you call this you have host or environment. Why you don't want to use it? #Closed

Ivanidzo4ka · 2018-09-24T23:21:13Z

src/Microsoft.ML.FastTree/FastTreeRegression.cs

+            string labelColumn,
+            string featureColumn,
+            string weightColumn = null,
+            int numLeaves = 20,


20 [](start = 28, length = 2)

Should it be part of static Defaults class? #Closed

Thanks.

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

You have Default class, but you don't use it here.

In reply to: 220063796 [](ancestors = 220063796,220018807)

Ivanidzo4ka · 2018-09-24T23:21:39Z

src/Microsoft.ML.FastTree/FastTreeStatic.cs

+    {
+        public static Scalar<float> FastTree(this RegressionContext.RegressionTrainers ctx,
+            Scalar<float> label, Vector<float> features, Scalar<float> weights = null,
+            int numLeaves = 20,


20 [](start = 28, length = 2)

Defaults? #Closed

Ivanidzo4ka · 2018-09-24T23:23:07Z

src/Microsoft.ML.StandardLearners/Standard/SdcaStatic.cs

-            Contracts.CheckParam(!(l1Threshold < 0), nameof(l1Threshold), "Must not be negative");
-            Contracts.CheckParam(!(maxIterations < 1), nameof(maxIterations), "Must be positive if specified");
+            Contracts.CheckParam(l2Const >= 0, nameof(l2Const), "Must not be negative");
+            Contracts.CheckParam(l1Threshold >= 0, nameof(l1Threshold), "Must not be negative");


l1Threshold >= 0 [](start = 33, length = 16)

How it works if l1Threshold is null? #Closed

Zruty0 · 2018-09-25T00:33:37Z

src/Microsoft.ML.FastTree/FastTree.cs

@@ -99,6 +99,13 @@ public abstract class FastTreeTrainerBase<TArgs, TTransformer, TModel> :

            //apply the advanced args, if the user supplied any
            advancedSettings?.Invoke(Args);
+
+            // check that the users didn't specify different label, group, feature, weights in the args, from what they supplied directly
+            TrainerUtils.CheckArgumesDefaultColNames(DefaultColumnNames.Label, Args.LabelColumn);


Argumes [](start = 30, length = 7)

typo #Resolved

Zruty0 · 2018-09-25T00:33:57Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

+            int numLeaves = 20,
+            int numTrees = 100,
+            int minDocumentsInLeafs = 10,
+            double learningRates = 0.2,


learningRates [](start = 19, length = 13)

just one learning rate will suffice #Resolved

i bet it is enough, but tried keeping them to the same names the args use :)

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

Zruty0 · 2018-09-25T00:36:11Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

+                var snapshot = new Arguments();
+
+                // Check that the user didn't supply different parameters in the args, from what it specified directly.
+                TrainerUtils.CheckArgsAndAdvancedSettingMismatch(numLeaves, snapshot.NumLeaves, Args.NumLeaves, nameof(numLeaves));


CheckArgsAndAdvancedSettingMismatch [](start = 29, length = 35)

actually this is not necessary. If they are so cool to set the values in 2 places, that's fine.
I wanted it to be checked on columns, since these are ignored. The other args, you set them first, then you invoke delegate and you're good. #Pending

well, this is trying to catch the case where they set one value in one place, and another one in the other. No exceptions, just Warn them about which value will be used.

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

Zruty0 · 2018-09-25T00:37:44Z

src/Microsoft.ML.FastTree/FastTreeStatic.cs

+            int minDocumentsInLeafs = 10,
+            double learningRates = 0.2,
+            Action<FastTreeBinaryClassificationTrainer.Arguments> advancedSettings = null,
+            Action<IPredictorWithFeatureWeights<float>> onFit = null)


IPredictorWithFeatureWeights [](start = 19, length = 28)

that's the best we could do here? I would argue that we should expose some form of tree ensemble predictor or something #Pending

I'll leave it for now and log an issue about it. Just casting didn't work.

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

Zruty0 · 2018-09-25T00:38:10Z

src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineStatic.cs

@@ -59,10 +59,10 @@ public static class FactorizationMachineStatic
                var trainer = new FieldAwareFactorizationMachineTrainer(env, labelCol, featureCols, advancedSettings:
                    args =>
                    {
+                        advancedSettings?.Invoke(args);


advancedSettings [](start = 24, length = 16)

my order was intentional :) #Pending

Zruty0

Assigning defaults to a few SDCA parameters.

sfilipi · 2018-09-25T07:03:37Z

src/Microsoft.ML.FastTree/FastTreeStatic.cs

+            int numLeaves = Defaults.NumLeaves,
+            int numTrees = Defaults.NumTrees,
+            int minDocumentsInLeafs = Defaults.MinDocumentsInLeafs,
+            double learningRate = Defaults.LearningRates,


fixxxx #Resolved

…arning-1 into FTPigstensions

Ivanidzo4ka · 2018-09-25T17:25:30Z

src/Microsoft.ML.FastTree/FastTreeStatic.cs

+{
+    public static class FastTreeStatic
+    {
+        public static Scalar<float> FastTree(this RegressionContext.RegressionTrainers ctx,


It's my time to comment what you need comments for public method! Oh, the joy of revenge!111 #Closed

I did them, before you commented!!!! see the next push :)

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

Ivanidzo4ka · 2018-09-25T17:26:33Z

test/Microsoft.ML.StaticPipelineTesting/Microsoft.ML.StaticPipelineTesting.csproj

@@ -7,13 +7,14 @@
    <ProjectReference Include="..\..\src\Microsoft.ML.ImageAnalytics\Microsoft.ML.ImageAnalytics.csproj" />
    <ProjectReference Include="..\..\src\Microsoft.ML.StandardLearners\Microsoft.ML.StandardLearners.csproj" />
    <ProjectReference Include="..\Microsoft.ML.TestFramework\Microsoft.ML.TestFramework.csproj" />
-    <NativeAssemblyReference Include="FactorizationMachineNative" />
-
+


EXTRA SPACES! #Closed

Ivanidzo4ka · 2018-09-25T17:27:27Z

test/Microsoft.ML.StaticPipelineTesting/Microsoft.ML.StaticPipelineTesting.csproj

    <ProjectReference Include="..\..\src\Microsoft.ML.Analyzer\Microsoft.ML.Analyzer.csproj">
      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
      <OutputItemType>Analyzer</OutputItemType>
    </ProjectReference>

    <NativeAssemblyReference Include="CpuMathNative" />
+    <NativeAssemblyReference Include="FactorizationMachineNative" />


I can see tests for FastTree added, but nothing regarding FM #Closed

it was already there. I just moved the reference a few lines down, to keep it together with the other native references.

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

Removing defaults from arguments of helper methods.

More common code set apart.

Ivanidzo4ka · 2018-09-25T18:19:20Z

test/Microsoft.ML.StaticPipelineTesting/Microsoft.ML.StaticPipelineTesting.csproj

-    <NativeAssemblyReference Include="LdaNative" />
+    <NativeAssemblyReference Include="FactorizationMachineNative" />
+    <NativeAssemblyReference Include="FastTreeNative" />
+	<NativeAssemblyReference Include="LdaNative" />


[](start = 0, length = 1)

tab #Closed

Tools->Options->Text Editor->All Languages->Insert Spaces

Checked!

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

Ivanidzo4ka · 2018-09-25T18:19:45Z

test/Microsoft.ML.StaticPipelineTesting/Microsoft.ML.StaticPipelineTesting.csproj

-    <ProjectReference Include="..\Microsoft.ML.TestFramework\Microsoft.ML.TestFramework.csproj" />
-    <NativeAssemblyReference Include="FactorizationMachineNative" />
-
+    <ProjectReference Include="..\Microsoft.ML.TestFramework\Microsoft.ML.TestFramework.csproj" />    


[](start = 98, length = 4)

is...
this...
a...
EXTRA SPACES? #Resolved

Ivanidzo4ka

…ion inside the advancedSettings, since they take precendence.

sfilipi added 2 commits September 24, 2018 15:05

FastTree classification and regression xtensions

67906db

adding tests and respective fixes

4b023b6

sfilipi added the API Issues pertaining the friendly API label Sep 24, 2018

sfilipi self-assigned this Sep 24, 2018

sfilipi requested review from TomFinley, Zruty0, artidoro, Ivanidzo4ka and zeahmed September 24, 2018 23:04

sfilipi commented Sep 24, 2018

View reviewed changes

Ivanidzo4ka reviewed Sep 24, 2018

View reviewed changes

Zruty0 reviewed Sep 25, 2018

View reviewed changes

Zruty0 approved these changes Sep 25, 2018

View reviewed changes

sfilipi added 3 commits September 24, 2018 23:23

addressing most of the code comments.

93751c9

Assigning defaults to a few SDCA parameters.

reverting to nullable types and setting defaults

3dc333a

one more to restore.

4d86fcf

sfilipi commented Sep 25, 2018

View reviewed changes

sfilipi added 2 commits September 25, 2018 09:59

merging from master

c9abb48

Merge branch 'FTPigstensions' of https://github.com/sfilipi/machinele…

1d6ade6

…arning-1 into FTPigstensions

Ivanidzo4ka reviewed Sep 25, 2018

View reviewed changes

sfilipi added 2 commits September 25, 2018 10:36

adding code comments, compacting common code.

538ecbf

Removing defaults from arguments of helper methods.

Using default values in FTRegression

7194439

More common code set apart.

Ivanidzo4ka reviewed Sep 25, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 25, 2018

View reviewed changes

sfilipi added 2 commits September 25, 2018 13:02

fixing the tests to use the direct parameters, rather than their vers…

ce9301b

…ion inside the advancedSettings, since they take precendence.

spaces.. and Ivan.

2cbcaba

sfilipi merged commit b270b4d into dotnet:master Sep 25, 2018

sfilipi deleted the FTPigstensions branch September 25, 2018 22:46

ghost locked as resolved and limited conversation to collaborators Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the extension methods for FastTree classification and regression #1009

Adding the extension methods for FastTree classification and regression #1009

sfilipi commented Sep 24, 2018

sfilipi Sep 24, 2018 •

edited

Loading

sfilipi Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Ivanidzo4ka Sep 25, 2018

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi Sep 25, 2018

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi Sep 25, 2018

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi Sep 25, 2018

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 left a comment

sfilipi Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Ivanidzo4ka Sep 25, 2018 •

edited by sfilipi

Loading

Ivanidzo4ka left a comment

Adding the extension methods for FastTree classification and regression #1009

Adding the extension methods for FastTree classification and regression #1009

Conversation

sfilipi commented Sep 24, 2018

sfilipi Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

sfilipi Sep 24, 2018 •

edited

Loading

sfilipi Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 24, 2018 •

edited

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited by sfilipi

Loading