Converting KMeans++trainer to estimator. #979

sfilipi · 2018-09-21T17:45:02Z

Ongoing work to address #754

sfilipi · 2018-09-21T17:55:34Z

test/Microsoft.ML.Tests/TrainerEstimators/TrainerEstimators.cs

@@ -16,5 +17,34 @@ public partial class TrainerEstimators : TestDataPipeBase
        public TrainerEstimators(ITestOutputHelper helper) : base(helper)
        {
        }
+
+        /// <summary>
+        /// FastTreeBinaryClassification TrainerEstimator test 


FastTreeBinaryClassification [](start = 12, length = 28)

fix comment #Resolved

sfilipi · 2018-09-21T17:56:33Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

@@ -203,6 +233,18 @@ private static int ComputeNumThreads(IHost host, int? argNumThreads)
            return Math.Max(1, maxThreads);
        }

+        private static SchemaShape.Column MakeWeightColumn(string weightColumn)


MakeWeightColumn [](start = 42, length = 16)

re-use from TrainerUtils #Resolved

sfilipi · 2018-09-21T17:58:01Z

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs

+    /// Base class for the <see cref="ISingleFeaturePredictionTransformer{TModel}"/> working on clustering tasks.
+    /// </summary>
+    /// <typeparam name="TModel">An implementation of the <see cref="IPredictorProducing{TResult}"/></typeparam>
+    public sealed class ClusteringPredictionTransformer<TModel> : SingleFeaturePredictionTransformerBase<TModel>


ClusteringPredictionTransformer [](start = 24, length = 31)

I still don't like not making those generic on the scorer as well... #Resolved

addressed on #996

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

Ivanidzo4ka · 2018-09-21T18:00:41Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

@@ -17,6 +17,7 @@
 using Microsoft.ML.Runtime.Training;
 using Microsoft.ML.Runtime.Internal.Internallearn;
 using Microsoft.ML.Runtime.EntryPoints;
+using Microsoft.ML.Core.Data;


sort order #Resolved

Ivanidzo4ka

Ivanidzo4ka · 2018-09-21T20:11:27Z

src/Microsoft.ML.Data/Scorers/PredictionTransformer.cs

+
+            // *** Binary format ***
+            // <base info>
+            // id of string: scorer threshold column


// id of string: scorer threshold column [](start = 11, length = 41)

doesn't look right #Resolved

…inelearning-1 into KmeansPCAEstimators

Zruty0 · 2018-09-24T21:37:41Z

test/Microsoft.ML.Benchmarks/KMeansAndLogisticRegressionBench.cs

@@ -52,9 +52,9 @@ public ParameterMixingCalibratedPredictor TrainKMeansAndLR()
                trans = TrainAndScoreTransform.Create(env, new TrainAndScoreTransform.Arguments
                {
                    Trainer = ComponentFactoryUtils.CreateFromFunction(host =>
-                        new KMeansPlusPlusTrainer(host, new KMeansPlusPlusTrainer.Arguments()
+                        new KMeansPlusPlusTrainer(host, "Features", advancedSettings: s=> 


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

I think K needs to be elevated out of 'advanced #Closed

Zruty0 · 2018-09-24T21:37:55Z

test/Microsoft.ML.TestFramework/Datasets.cs

        };

        public static TestDataset mnistTiny28 = new TestDataset()
        {
            name = "mnistTiny28",
-            trainFilename = @"..\MNIST\Train-Tiny-28x28.txt",
-            testFilename = @"..\MNIST\Test-Tiny-28x28.txt"
+            trainFilename = @"Train-Tiny-28x28.txt",


Train [](start = 30, length = 5)

intentional? #Closed

yes. The 'old' path is from the TLC solution structure.

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

Zruty0 · 2018-09-24T21:38:30Z

test/Microsoft.ML.Tests/TrainerEstimators/TrainerEstimators.cs

+                            advancedSettings: s => { s.InitAlgorithm = KMeansPlusPlusTrainer.InitAlgorithm.KMeansParallel; });
+
+            TestEstimatorCore(pipeline, data);
+        }


} [](start = 8, length = 1)

Call Done() at the end #Closed

Zruty0 · 2018-09-24T21:39:10Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

+        /// </summary>
+        /// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
+        /// <param name="featureColumn">The name of the feature column.</param>
+        /// <param name="weightColumn">The name for the column containing the initial weight.</param>


initial [](start = 78, length = 7)

example weights, not initial weight #Closed

Zruty0 · 2018-09-24T21:39:45Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

            Host.CheckUserArg(args.K > 0, nameof(args.K), "Must be positive");

+            // is this even necessary, if there is only one column, for example


is this even necessary, if there is only one column, for example [](start = 15, length = 64)

It checks for non-emptiness of string #Closed

Zruty0 · 2018-09-24T21:41:15Z

This calls for a ClusteringContext with a pigstension for KMeans #Closed

Zruty0

🕐

TomFinley · 2018-09-25T16:50:37Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

        {
-            Host.CheckValue(args, nameof(args));
+            if (args == null)
+                args = new Arguments();


The net effect of this change is that the internal constructor is now tolerant to passing in null arguments.. Even though the constructor is internal I'd prefer to keep it clean, and maintain the invariants we have everywhere else. Note the Host.CheckValue(args, nameof(args)); check that was deleted.

If you still had the check, and just passed in new Arguments in the above constructor, that would have had no more lines of code. #Resolved

appreciate pointing this out.

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

TomFinley · 2018-09-25T16:51:44Z

using Microsoft.ML.Runtime;

I like how when you changed from Float to float you made those two separate commits. That makes things easier to check out. #Resolved

Refers to: src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs:10 in bd436bb. [](commit_id = bd436bb, deletion_comment = True)

sfilipi · 2018-09-25T20:27:49Z

using Microsoft.ML.Runtime;

thanks for pointing out. Will keep that in mind.

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

Refers to: src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs:10 in bd436bb. [](commit_id = bd436bb, deletion_comment = True)

elevating K to constructor param

…d, the KMeans extension on the clustering context and a tet for it.

Zruty0 · 2018-09-27T16:23:13Z

src/Microsoft.ML.Data/Evaluators/ClusteringEvaluator.cs

+        /// <param name="predictedLabel">The name of the predicted label column in <paramref name="data"/>.</param>
+        /// <param name="features">The name of the feature column in <paramref name="data"/>.</param>
+        /// <returns>The evaluation results.</returns>
+        public Result Evaluate(IDataView data, string label, string score, string predictedLabel, string features = null)


label [](start = 54, length = 5)

what is 'label' and why is it required? Or 'predictedLabel' for that matter? #Resolved

OK, I double-checked. label is required for NMI, if it's not provided, NMI is not calculated. That makes sense. Let's make it optional.
predictedLabel is never required, as I suspected. Let's remove it.

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

Zruty0

Turning the label column into an optional one.

sfilipi added 2 commits September 20, 2018 23:56

Kmeans to estimator.

7a3c505

Code comments, adding the test

47271db

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

sfilipi self-assigned this Sep 21, 2018

sfilipi requested review from Ivanidzo4ka, TomFinley, artidoro and Zruty0 September 21, 2018 17:45

Merge branch 'master' into KmeansPCAEstimators

bd436bb

Zruty0 mentioned this pull request Sep 21, 2018

New API for ML.NET #754

Closed

sfilipi commented Sep 21, 2018

View reviewed changes

Ivanidzo4ka reviewed Sep 21, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 21, 2018

View reviewed changes

Ivanidzo4ka reviewed Sep 21, 2018

View reviewed changes

sfilipi added 7 commits September 21, 2018 15:30

code comment and swap System.Single for float

a80438f

merging from master

abc561c

merge with master

c426f22

merging from master

021d24b

Addressing code comments.

b622a35

Merge branch 'KmeansPCAEstimators' of https://github.com/sfilipi/mach…

45414d3

…inelearning-1 into KmeansPCAEstimators

post merge fix.

7bddcac

Zruty0 reviewed Sep 24, 2018

View reviewed changes

Zruty0 suggested changes Sep 24, 2018

View reviewed changes

TomFinley reviewed Sep 25, 2018

View reviewed changes

Merge branch 'master' into KmeansPCAEstimators

24557d5

sfilipi added 5 commits September 25, 2018 13:38

fixing the merge with the component catalog change

2b7c3b6

elevating K to constructor param

Adding the Clustering training context, the clustering Evaluate metho…

3de277b

…d, the KMeans extension on the clustering context and a tet for it.

Fixing all comments.

6649964

merging from master

7272a8e

post merge updates

e6f9197

Zruty0 reviewed Sep 27, 2018

View reviewed changes

Zruty0 approved these changes Sep 27, 2018

View reviewed changes

sfilipi added 2 commits September 27, 2018 22:18

Removing the predictedLabel from the Evaluation methods.

2b6df89

Turning the label column into an optional one.

Merge branch 'master' into KmeansPCAEstimators

0cf03a5

sfilipi merged commit 8aa4f1f into dotnet:master Sep 28, 2018

sfilipi deleted the KmeansPCAEstimators branch September 28, 2018 16:29

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

Converting KMeans++trainer to estimator. #979

Converting KMeans++trainer to estimator. #979

sfilipi commented Sep 21, 2018

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 24, 2018

Ivanidzo4ka Sep 21, 2018 •

edited by sfilipi

Loading

Ivanidzo4ka left a comment

Ivanidzo4ka Sep 21, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

sfilipi Sep 25, 2018

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 commented Sep 24, 2018 •

edited

Loading

Zruty0 left a comment

TomFinley Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi Sep 25, 2018

TomFinley commented Sep 25, 2018 •

edited by sfilipi

Loading

sfilipi commented Sep 25, 2018

Zruty0 Sep 27, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 27, 2018

Zruty0 left a comment

		Host.CheckUserArg(args.K > 0, nameof(args.K), "Must be positive");

		// is this even necessary, if there is only one column, for example

Converting KMeans++trainer to estimator. #979

Converting KMeans++trainer to estimator. #979

Conversation

sfilipi commented Sep 21, 2018

sfilipi Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 24, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Sep 21, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 21, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 25, 2018

Choose a reason for hiding this comment

Zruty0 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Sep 24, 2018 • edited Loading

Zruty0 left a comment

Choose a reason for hiding this comment

TomFinley Sep 25, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Sep 25, 2018

Choose a reason for hiding this comment

TomFinley commented Sep 25, 2018 • edited by sfilipi Loading

sfilipi commented Sep 25, 2018

Zruty0 Sep 27, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 Sep 27, 2018

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 21, 2018 •

edited

Loading

Ivanidzo4ka Sep 21, 2018 •

edited by sfilipi

Loading

Ivanidzo4ka Sep 21, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 Sep 24, 2018 •

edited

Loading

Zruty0 commented Sep 24, 2018 •

edited

Loading

TomFinley Sep 25, 2018 •

edited by sfilipi

Loading

TomFinley commented Sep 25, 2018 •

edited by sfilipi

Loading

Zruty0 Sep 27, 2018 •

edited by sfilipi

Loading