Scrubbing FieldAwareFactorizationMachine learner. #2730

zeahmed · 2019-02-26T01:06:51Z

This PR partially addressed #2620

In FieldAwareFactorizationMachine related classes, the following tasks were performed.

Checking to make sure that unnecessary public methods/properties be internal.
Renaming parameters according to standard.
Creating/Refactoring samples according to standards.

Ivanidzo4ka · 2019-02-26T01:12:04Z

src/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs

@@ -78,14 +78,48 @@ public sealed class HousingRegression
        /// <summary>
        /// Downloads the wikipedia detox dataset from the ML.NET repo.
        /// </summary>
-        public static string DownloadSentimentDataset()
-         => Download("https://github.com/raw/dotnet/machinelearning/76cb2cdf5cc8b6c88ca44b8969153836e589df04/test/data/wikipedia-detox-250-line-data.tsv", "sentiment.tsv");
+        public static (string trainFile, string testFile) DownloadSentimentDataset()


(string trainFile, string testFile) [](start = 22, length = 35)

#2501

Can we not use value tuples? They nice, but are they really necessary?
Imagine we will need to write samples to F#, and we want to reuse this class. Would be quite bad. #Resolved

Ivanidzo4ka · 2019-02-26T01:13:13Z

        public int Iters = 5;

Samples are good, but I would prefer you to focus on public surface. #Resolved

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:51 in 19d25e2. [](commit_id = 19d25e2, deletion_comment = False)

Ivanidzo4ka · 2019-02-26T01:13:22Z

        public int LatentDim = 20;

brrr #Resolved

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:55 in 19d25e2. [](commit_id = 19d25e2, deletion_comment = False)

Ivanidzo4ka · 2019-02-26T01:13:45Z

        public bool Norm = true;

brr #Resolved

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:66 in 19d25e2. [](commit_id = 19d25e2, deletion_comment = False)

Ivanidzo4ka · 2019-02-26T01:15:12Z

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

@@ -156,12 +156,12 @@ internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env, Options opt
        /// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
        /// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>
        /// <param name="labelColumn">The name of the label column.</param>
-        /// <param name="weights">The name of the optional weights' column.</param>
+        /// <param name="weightColumn">The name of the optional weights' column.</param>


/// The name of the optional weights' column. [](start = 7, length = 81)

/// <param name="exampleWeightColumnName">The name of the example weight column (optional).</param> #Resolved

Ivanidzo4ka · 2019-02-26T01:17:34Z

...Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachine.cs

+    {
+        public static void Example()
+        {
+            // Creating the ML.Net IHostEnvironment object, needed for the pipeline.


// Creating the ML.Net IHostEnvironment object, needed for the pipeline. [](start = 12, length = 72)

// Create a new context for ML.NET operations. It can be used for exception tracking and logging, // as a catalog of available operations and as the source of randomness.

I think this is our default comment. #Resolved

Ivanidzo4ka · 2019-02-26T01:18:24Z

...Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachine.cs

+            // Let's inspect the model parameters.
+            var featureCount = modelParams.GetFeatureCount();
+            var fieldCount = modelParams.GetFieldCount();
+            var latentDim = modelParams.GetLatentDim();


GetLatentDim [](start = 40, length = 12)

brrr #Resolved

Ivanidzo4ka · 2019-02-26T01:23:20Z

...Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachine.cs

+            var featureCount = modelParams.GetFeatureCount();
+            var fieldCount = modelParams.GetFieldCount();
+            var latentDim = modelParams.GetLatentDim();
+            var linearWeights = modelParams.GetLinearWeights();


var linearWeights = modelParams.GetLinearWeights(); [](start = 12, length = 51)

I think right now I can do following:
Evaluate(model.Transform(Data)) -> AUC = X1

var linearWeights = modelParams.GetLinearWeights(); linearWeights[0] =100; linearWeights[1] =200;

Evaluate(model.Transform(Data)) -> AUC = X2

X1 != X2

Which is awful.

Can you check MatrixFactoraztionPredictor and how it handles arrays?

I also don't understand GetFeatureCoun() functions, why can't I just do modelParams.FeatureCount?
#Resolved

Ivanidzo4ka · 2019-02-26T01:35:06Z

        public float LearningRate = (float)0.1;

you can at least copy description from HelpText to summary for this fields. #Resolved

Refers to: src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs:47 in 19d25e2. [](commit_id = 19d25e2, deletion_comment = False)

sfilipi · 2019-02-26T06:07:27Z

...Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachine.cs

+            // the "Features" column as the features column.
+            var pipeline = new EstimatorChain<ITransformer>().AppendCacheCheckpoint(mlContext)
+                .Append(mlContext.BinaryClassification.Trainers.
+                FieldAwareFactorizationMachine(labelColumnName: "Sentiment", featureColumnNames: new[] { "Features" }));


indentation #Resolved

sfilipi · 2019-02-26T06:08:49Z

....Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachinewWithOptions.cs

+            // the "Features" column as the features column.
+            var pipeline = new EstimatorChain<ITransformer>().AppendCacheCheckpoint(mlContext)
+                .Append(mlContext.BinaryClassification.Trainers.
+                FieldAwareFactorizationMachine(


one more #Resolved

wschin · 2019-02-26T16:57:43Z

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

@@ -26,7 +26,7 @@ namespace Microsoft.ML.FactorizationMachine
 {
    /*
     Train a field-aware factorization machine using ADAGRAD (an advanced stochastic gradient method). See references below
-     for details. This trainer is essentially faster the one introduced in [2] because of some implementation tricks[3].
+     for details. This trainer is essentially faster than the one introduced in [2] because of some implementation tricks[3].


Suggested change

for details. This trainer is essentially faster than the one introduced in [2] because of some implementation tricks[3].

for details. This trainer is essentially faster than the one introduced in [2] because of some implementation tricks in [3].

``` #Resolved

wschin · 2019-02-26T16:58:17Z

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

@@ -156,12 +156,12 @@ internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env, Options opt
        /// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
        /// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>


Suggested change

/// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>

/// <param name="featureColumnNames">The names of columns hosting the features. The i-th element stores feature column of the i-th field.</param>

``` #Resolved

wschin · 2019-02-26T16:58:32Z

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

@@ -156,12 +156,12 @@ internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env, Options opt
        /// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
        /// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>
        /// <param name="labelColumn">The name of the label column.</param>


Suggested change

/// <param name="labelColumn">The name of the label column.</param>

/// <param name="labelColumnName">The name of the label column.</param>

``` #Resolved

…arning into learners_check

codecov · 2019-02-26T22:06:15Z

Codecov Report

Merging #2730 into master will decrease coverage by 0.02%.
The diff coverage is 51.42%.

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
- Coverage   71.67%   71.65%   -0.03%     
==========================================
  Files         808      808              
  Lines      142364   142382      +18     
  Branches    16121    16121              
==========================================
- Hits       102043   102023      -20     
- Misses      35879    35921      +42     
+ Partials     4442     4438       -4

Flag	Coverage Δ
#Debug	`71.65% <51.42%> (-0.03%)`	⬇️
#production	`67.9% <48.48%> (-0.03%)`	⬇️
#test	`85.83% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
...c/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs	`23.9% <0%> (-1.3%)`	⬇️
...rosoft.ML.Tests/TrainerEstimators/FAFMEstimator.cs	`100% <100%> (ø)`	⬆️
...actorizationMachine/FactorizationMachineCatalog.cs	`50% <50%> (ø)`	⬆️
...rosoft.ML.StaticPipe/FactorizationMachineStatic.cs	`71.11% <50%> (ø)`	⬆️
...e/FieldAwareFactorizationMachineModelParameters.cs	`75.59% <63.15%> (+0.88%)`	⬆️
...actorizationMachine/FactorizationMachineTrainer.cs	`88.18% <90%> (ø)`	⬆️
src/Microsoft.ML.Core/Data/ProgressReporter.cs	`70.95% <0%> (-6.99%)`	⬇️
src/Microsoft.ML.Maml/MAML.cs	`24.75% <0%> (-1.46%)`	⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`82.86% <0%> (+0.2%)`	⬆️
...StandardLearners/Standard/LinearModelParameters.cs	`60.9% <0%> (+0.26%)`	⬆️

wschin · 2019-02-26T22:09:19Z

....Samples/Dynamic/Trainers/BinaryClassification/FieldAwareFactorizationMachinewWithOptions.cs

+            Console.WriteLine("The number of fields is: " + fieldCount);
+            Console.WriteLine("The latent dimension is: " + latentDim);
+            Console.WriteLine("The linear weights of some of the features are: " +
+                string.Concat(Enumerable.Range(1, 10).Select(i => $"{linearWeights[i]:F4} ")));


string.Concats are not aligned. #Resolved

wschin

Ivanidzo4ka · 2019-02-26T22:37:25Z

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

            [TlcModule.SweepableLongParam(1, 100)]
-            public int Iters = 5;
+            public int Iterations = 5;


NumberOfIterations for consistency with other learners. #Resolved

Ivanidzo4ka · 2019-02-26T22:37:32Z

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

+            /// <summary>
+            /// Number of training iterations.
+            /// </summary>
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Number of training iterations", ShortName = "iters", SortOrder = 2)]


iters [](start = 104, length = 5)

iters,iter #Resolved

wschin · 2019-02-26T22:45:56Z

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

-        /// <param name="weights">The name of the optional weights' column.</param>
+        /// <param name="featureColumnNames">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>
+        /// <param name="labelColumnName">The name of the label column.</param>
+        /// <param name="weightColumnName">The name of the weight column (optional).</param>
        [BestFriend]
        internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env,


Suggested change

internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env,

internal FieldAwareFactorizationMachineBinaryClassificationTrainer(IHostEnvironment env,

``` #Resolved

Ivanidzo4ka · 2019-02-26T22:46:49Z

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

        /// <summary>
        /// The linear coefficients of the features. It's the symbol `w` in the doc: https://github.com/wschin/fast-ffm/blob/master/fast-ffm.pdf
        /// </summary>
-        public float[] GetLinearWeights() => _linearWeights;
+        public float[] GetLinearWeights()


float[] [](start = 15, length = 7)

I would reach Tom and double check, but I think we don't want expose arrays. We want expose immutable collections, either ImmutableArray or IReadOnlyList not sure which we prefer.
If you expose array, that
a) force you to copy data.
b) give user idea what he can change it. (which is not true)
#Resolved

In tree's public interface, we expose IReadOnlyList for all array attributes. #Resolved

What about ReadOnlyCollection? Any idea @TomFinley? what we are following?

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

OK, I follow what @wschin suggest. IReadOnlyList #Resolved

wschin · 2019-02-27T00:13:20Z

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

@@ -253,11 +253,9 @@ internal void CopyLatentWeightsTo(AlignedArray latentWeights)
        /// <summary>
        /// The linear coefficients of the features. It's the symbol `w` in the doc: https://github.com/wschin/fast-ffm/blob/master/fast-ffm.pdf
        /// </summary>
-        public float[] GetLinearWeights()
+        public IReadOnlyList<float> GetLinearWeights()


One line function should look like public IReadOnlyList<float> Get() => _linearWeights. #Resolved

Ivanidzo4ka

Scrubbing FieldAwareFactorizationMachine learner.

19d25e2

zeahmed requested review from Ivanidzo4ka, shmoradims and sfilipi February 26, 2019 01:06

Ivanidzo4ka reviewed Feb 26, 2019

View reviewed changes

sfilipi reviewed Feb 26, 2019

View reviewed changes

wschin reviewed Feb 26, 2019

View reviewed changes

zeahmed added 6 commits February 26, 2019 12:46

Addressed reviewers' comments.

b89c63b

Fixed entrypoint json.

0bc0c92

Fixed entrypoint json.

4051d6b

Merge branch 'learners_check' of https://github.com/zeahmed/machinele…

17f83a9

…arning into learners_check

Resolved merged conflicts.

7d4e7ed

Resolved a compilation bug.

3f7cf53

wschin reviewed Feb 26, 2019

View reviewed changes

wschin approved these changes Feb 26, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 26, 2019

View reviewed changes

wschin reviewed Feb 26, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 26, 2019

View reviewed changes

Addressed reviewers' comments.

30faca7

wschin reviewed Feb 27, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 27, 2019

View reviewed changes

zeahmed added 2 commits February 26, 2019 16:40

Resolved merged conflicts.

c2746e2

Addressed reviewers' comments.

003b6a0

zeahmed merged commit 482bb81 into dotnet:master Feb 27, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scrubbing FieldAwareFactorizationMachine learner. #2730

Scrubbing FieldAwareFactorizationMachine learner. #2730

zeahmed commented Feb 26, 2019

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

sfilipi Feb 26, 2019 •

edited by zeahmed

Loading

sfilipi Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

codecov bot commented Feb 26, 2019 •

edited

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

wschin left a comment

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

zeahmed Feb 26, 2019 •

edited

Loading

zeahmed Feb 26, 2019 •

edited

Loading

wschin Feb 27, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka left a comment

	for details. This trainer is essentially faster than the one introduced in [2] because of some implementation tricks[3].
	for details. This trainer is essentially faster than the one introduced in [2] because of some implementation tricks in [3].
	``` #Resolved

		@@ -156,12 +156,12 @@ internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env, Options opt
		/// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
		/// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>

	/// <param name="featureColumns">The name of column hosting the features. The i-th element stores feature column of the i-th field.</param>
	/// <param name="featureColumnNames">The names of columns hosting the features. The i-th element stores feature column of the i-th field.</param>
	``` #Resolved

	/// <param name="labelColumn">The name of the label column.</param>
	/// <param name="labelColumnName">The name of the label column.</param>
	``` #Resolved

	internal FieldAwareFactorizationMachineTrainer(IHostEnvironment env,
	internal FieldAwareFactorizationMachineBinaryClassificationTrainer(IHostEnvironment env,
	``` #Resolved

Scrubbing FieldAwareFactorizationMachine learner. #2730

Scrubbing FieldAwareFactorizationMachine learner. #2730

Conversation

zeahmed commented Feb 26, 2019

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 26, 2019 • edited by zeahmed Loading

Ivanidzo4ka commented Feb 26, 2019 • edited by zeahmed Loading

Ivanidzo4ka commented Feb 26, 2019 • edited by zeahmed Loading

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 26, 2019 • edited by zeahmed Loading

sfilipi Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

sfilipi Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2019 • edited Loading

Codecov Report

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

wschin Feb 26, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

zeahmed Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

zeahmed Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

wschin Feb 27, 2019 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by zeahmed

Loading

sfilipi Feb 26, 2019 •

edited by zeahmed

Loading

sfilipi Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

codecov bot commented Feb 26, 2019 •

edited

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by zeahmed

Loading

wschin Feb 26, 2019 •

edited by zeahmed

Loading

zeahmed Feb 26, 2019 •

edited

Loading

zeahmed Feb 26, 2019 •

edited

Loading

wschin Feb 27, 2019 •

edited by zeahmed

Loading