Internalize and cleanup recommender project #2451

Ivanidzo4ka · 2019-02-07T02:37:40Z

This reverts commit 9138979.

codecov · 2019-02-07T03:14:27Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2451   +/-   ##
=========================================
  Coverage          ?   71.21%           
=========================================
  Files             ?      786           
  Lines             ?   140989           
  Branches          ?    16114           
=========================================
  Hits              ?   100408           
  Misses            ?    36112           
  Partials          ?     4469

Flag	Coverage Δ
#Debug	`71.21% <76.06%> (?)`
#production	`67.57% <70.21%> (?)`
#test	`85.3% <100%> (?)`

Ivanidzo4ka · 2019-02-07T22:18:38Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

+        /// <summary>
+        /// Returns right approximation matrix.
+        /// </summary>
+        /// <returns>Flattened into one dimension array with size equal to <see cref="ApproximationRank"/> * <see cref="NumberOfColumns"/></returns>


dimension [](start = 40, length = 9)

dimensional #Closed

Ivanidzo4ka · 2019-02-07T22:26:35Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs


+            /// <summary>
+            /// Approximation rank.


Approximation rank. [](start = 16, length = 19)

Rank of approximation matrixes? #Closed

wschin · 2019-02-07T23:21:12Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

-        private readonly int _approximationRank;
-        // Packed _numberOfRows by _approximationRank matrix.
+        ///<summary> The number of rows.</summary>
+        public readonly int NumberOfRows;


Is one line summary a new convention? I remember we used to have

<summary> ... </summary> ``` #Resolved

You can still use old one. This one is nice if you have short description.

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

wschin · 2019-02-07T23:23:34Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

+        /// Returns left approximation matrix.
+        /// </summary>
+        /// <returns>Flattened into one-dimensional array with size equal to <see cref="NumberOfRows"/> * <see cref="ApproximationRank"/></returns>
+        public float[] GetLeftFactorMatrix()
        {


Any test for the two public functions, GetLeftFactorMatrix and GetRightFactorMatrix? #Resolved

yes, I modified one of tests see MatrixFactorizationTests.cs

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

wschin · 2019-02-07T23:24:46Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

+        /// Returns left approximation matrix.
+        /// </summary>
+        /// <returns>Flattened into one-dimensional array with size equal to <see cref="NumberOfRows"/> * <see cref="ApproximationRank"/></returns>
+        public float[] GetLeftFactorMatrix()


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

Do we want to use IReadOnlyList to avoid active copy? #Resolved

wschin · 2019-02-07T23:25:37Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs


-        public ColumnType OutputType { get { return NumberType.Float; } }
+        private ColumnType OutputType { get { return NumberType.Float; } }


{ get { return NumberType.Float; } } [](start = 38, length = 36)

Maybe => NumberType.Float? #Resolved

wschin · 2019-02-07T23:30:59Z

We have a public Save here. Do we have a public Load or something similar? #Resolved

Refers to: src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs:167 in 67e9f4d. [](commit_id = 67e9f4d, deletion_comment = False)

wschin · 2019-02-07T23:32:58Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs


-        internal MatrixFactorizationPredictor(IHostEnvironment env, SafeTrainingAndModelBuffer buffer, KeyType matrixColumnIndexType, KeyType matrixRowIndexType)
+        internal MatrixFactorizationModelParameters(IHostEnvironment env, SafeTrainingAndModelBuffer buffer, KeyType matrixColumnIndexType, KeyType matrixRowIndexType)
        {


Do we have a public constructor for creating a MatrixFactorizationModelParameters from its public fields? #Resolved

No we don't.
See #1866

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

wschin · 2019-02-07T23:42:38Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// <remarks>
+            /// Two values are allowed, <see cref="LossFunctionType.SquareLossRegression"/> or <see cref="LossFunctionType.SquareLossOneClass"/>.
+            /// The <see cref="LossFunctionType.SquareLossRegression"/> means traditional collaborative filtering problem with squared loss.
+            /// The <see cref="LossFunctionType.SquareLossOneClass"/> triggers one-class matrix factorization for implicit-feedback recommendation problem.


Please add For the first case, please see Equation (1) in [https://www.csie.ntu.edu.tw/~cjlin/papers/libmf/mf_adaptive_pakdd.pdf](https://www.csie.ntu.edu.tw/~cjlin/papers/libmf/mf_adaptive_pakdd.pdf) for the formulation considered. For the second case, please see Equation (3) in [http://yifanhu.net/PUB/cf.pdf](http://yifanhu.net/PUB/cf.pdf`). #Closed

I'll add that to loss functions

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

Can you check them as remarks on loss functions enum?

In reply to: 254911715 [](ancestors = 254911715,254911243)

wschin · 2019-02-07T23:43:29Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// </summary>
+            /// <remarks>
+            /// It's the weight of factor matrices' norms in the objective function minimized by matrix factorization's algorithm. A small value could cause over-fitting.
+            /// </remarks>
            [Argument(ArgumentType.AtMostOnce, HelpText = "Regularization parameter. " +
                "It's the weight of factor matrices' norms in the objective function minimized by matrix factorization's algorithm. " +


norms [](start = 53, length = 5)

Frobenius Norm #Resolved

Are you trying to summon demon in my PR?

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

wschin · 2019-02-07T23:43:54Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// Rank of approximation matrixes.
+            /// </summary>
+            /// <remarks>
+            /// If input data has size of m by n we would build two approximation matrixes m by k and k by n where k is approxiation rank.


m by n [](start = 42, length = 6)

m-by-n #Resolved

wschin · 2019-02-07T23:44:25Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// Rank of approximation matrixes.
+            /// </summary>
+            /// <remarks>
+            /// If input data has size of m by n we would build two approximation matrixes m by k and k by n where k is approxiation rank.


m by k [](start = 91, length = 6)

m-by-k #Resolved

wschin · 2019-02-07T23:44:35Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// Rank of approximation matrixes.
+            /// </summary>
+            /// <remarks>
+            /// If input data has size of m by n we would build two approximation matrixes m by k and k by n where k is approxiation rank.


k by n [](start = 102, length = 6)

k-by-n #Resolved

wschin · 2019-02-07T23:44:54Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// Rank of approximation matrixes.
+            /// </summary>
+            /// <remarks>
+            /// If input data has size of m by n we would build two approximation matrixes m by k and k by n where k is approxiation rank.


approxiation [](start = 120, length = 12)

approximation. #Resolved

wschin · 2019-02-07T23:46:44Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs


+            /// <summary>
+            /// Number of thread will be used during training. If unspecified all aviable threads will be use.


thread [](start = 26, length = 6)

threads #Resolved

wschin · 2019-02-07T23:51:00Z

src/Microsoft.ML.Recommender/RecommenderCatalog.cs

+            /// <![CDATA[
+            ///  [!code-csharp[MatrixFactorization](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Recommendation/MatrixFactorization.cs)]
+            /// ]]></format>
+            /// </example>
            public MatrixFactorizationTrainer MatrixFactorization(


MatrixFactorization [](start = 46, length = 19)

Loss function is an important parameter. Could we have it here? #Resolved

as soon as I add loss function I need to add rest of option parameters.
We have another overload with options, if user want to specify loss functions, he can use it.

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

Ivanidzo4ka · 2019-02-08T00:02:39Z

@artidoro has PR which would tackle public Save methods.

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

Refers to: src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs:167 in 67e9f4d. [](commit_id = 67e9f4d, deletion_comment = False)

wschin · 2019-02-08T00:17:33Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

-            _host.Assert(_leftFactorMatrix.Length == _numberOfRows * _approximationRank);
-            _host.Assert(_rightFactorMatrix.Length == _numberofColumns * _approximationRank);
+            buffer.Get(out NumberOfRows, out NumberOfColumns, out ApproximationRank, out var leftFactorMatrix, out var rightFactorMatrix);
+            LeftFactorMatrix = Array.AsReadOnly<float>(leftFactorMatrix);


Array.AsReadOnly [](start = 31, length = 23)

Not sure if casting is necessary. #Resolved

wschin

LGTM.

wschin · 2019-02-08T18:18:02Z

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

+            var rightMatrix = model.Model.RightFactorMatrix;
+            Assert.Equal(leftMatrix.Count, model.Model.NumberOfRows * model.Model.ApproximationRank);
+            Assert.Equal(rightMatrix.Count, model.Model.NumberOfColumns * model.Model.ApproximationRank);
+


At least Assert.Equal() the first and the last elements in the returned arrays. #Resolved

wschin · 2019-02-08T18:18:21Z

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

            };

            var pipeline = mlContext.Recommendation().Trainers.MatrixFactorization(options);

            // Train a matrix factorization model.
            var model = pipeline.Fit(data);

+            // Les's validate content of the model.


Les's [](start = 15, length = 5)

Let's #Resolved

artidoro · 2019-02-08T18:46:23Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

-        public ColumnType MatrixColumnIndexType { get; }
-        public ColumnType MatrixRowIndexType { get; }
+        internal ColumnType MatrixColumnIndexType { get; }
+        internal ColumnType MatrixRowIndexType { get; }


I think in Tom's last internalization PR, he left the methods that return the types of the input/output matrices public.
It could make sense that one wants to access that after, say after having loaded a previously trained model to know what type of data it can score. #Resolved

Tom PR wasn't about INTENALIZE EVERYTHING I CAN SEE, right?
I ask him and this is his reply:

For now probably internal. Unless you have some scenario that requires it.

I don't see such scenarios, @wschin do you see such?

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

Fair enough :)

In reply to: 255211305 [](ancestors = 255211305,255193733)

Thank you for asking Tom!

In reply to: 255211957 [](ancestors = 255211957,255211305,255193733)

artidoro · 2019-02-08T18:50:14Z

src/Microsoft.ML.Recommender/MatrixFactorizationPredictor.cs

+        internal string MatrixColumnIndexColumnName { get; }
+        internal string MatrixRowIndexColumnName { get; }
+        internal ColumnType MatrixColumnIndexColumnType { get; }
+        internal ColumnType MatrixRowIndexColumnType { get; }


Same remark here. Although I am not sure if it makes sense to make it available in both places? #ByDesign

artidoro · 2019-02-08T18:51:16Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

+            /// See <a href="http://yifanhu.net/PUB/cf.pdf">Equation</a> (3).
+            /// </remarks>
+            SquareLossOneClass = 12
+        };

        public sealed class Options


public [](start = 8, length = 6)

Can we have a line of xml here too? #Resolved

artidoro · 2019-02-08T18:54:05Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

@@ -403,7 +483,7 @@ private SafeTrainingAndModelBuffer PrepareBuffer()
        /// <param name="validationData">The validation data set.</param>
        public MatrixFactorizationPredictionTransformer Train(IDataView trainData, IDataView validationData = null)
        {


Can we make this private? Or internal? #ByDesign

No, this one needed for the case when you train with validation data.

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

My bad thank you!

In reply to: 255197330 [](ancestors = 255197330,255196397)

artidoro

Ivanidzo4ka · 2019-02-08T20:28:10Z

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

+            var rightMatrix = model.Model.RightFactorMatrix;
+            Assert.Equal(leftMatrix.Count, model.Model.NumberOfRows * model.Model.ApproximationRank);
+            Assert.Equal(rightMatrix.Count, model.Model.NumberOfColumns * model.Model.ApproximationRank);
+            Assert.Equal(leftMatrix[0], (double)0.3091519,5);


leftMatrix[0] [](start = 25, length = 13)

@wschin do you know why I get
on Mac:
Expected: 0.41953 (rounded from 0.419525027275085)
Actual: 0.30915 (rounded from 0.3091519)
on Linux:
Expected: 0.51506 (rounded from 0.515059947967529)
Actual: 0.30915 (rounded from 0.3091519)

and how you handle that discrepancy between mac/linux and windows before?

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

Ivan Matantsev added 9 commits February 3, 2019 13:45

Fix build

9138979

Revert "Fix build"

e59f450

This reverts commit 9138979.

Merge remote-tracking branch 'upstream/master'

2d9cd78

Merge remote-tracking branch 'upstream/master'

9cbb55e

Merge remote-tracking branch 'upstream/master'

7aab754

Merge remote-tracking branch 'upstream/master'

6755276

Merge remote-tracking branch 'upstream/master'

cf639cc

Merge remote-tracking branch 'upstream/master'

d01bd83

Lockdown Microsoft.ML.Recommender public surface

8f8f8ca

Documentation, ModelParameters

9496ca2

Ivanidzo4ka requested review from wschin and artidoro February 7, 2019 22:08

Ivanidzo4ka changed the title ~~[WIP] Internalize recommender project~~ Internalize recommender project Feb 7, 2019

merge with master

8806dac

Ivanidzo4ka commented Feb 7, 2019

View reviewed changes

Ivanidzo4ka changed the title ~~Internalize recommender project~~ Internalize and cleanup recommender project Feb 7, 2019

Ivanidzo4ka requested a review from TomFinley February 7, 2019 22:28

minor changes + make it pass tests.

67e9f4d

wschin reviewed Feb 7, 2019

View reviewed changes

Ivan Matantsev added 2 commits February 7, 2019 16:04

address feedback

d42ed8a

clean loss remarks

c7e7f22

wschin reviewed Feb 8, 2019

View reviewed changes

wschin approved these changes Feb 8, 2019

View reviewed changes

remove unnecessary casting

90b0292

Ivanidzo4ka requested a review from sfilipi February 8, 2019 01:20

wschin reviewed Feb 8, 2019

View reviewed changes

artidoro reviewed Feb 8, 2019

View reviewed changes

artidoro approved these changes Feb 8, 2019

View reviewed changes

address comments

cd594e0

Ivanidzo4ka mentioned this pull request Feb 8, 2019

Lockdown of Microsoft.ML.LightGBM public surface. #2476

Merged

Ivanidzo4ka commented Feb 8, 2019

View reviewed changes

test matrix content only on windows

c7e4e3a

Ivanidzo4ka merged commit 48dffb0 into dotnet:master Feb 8, 2019

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


		public ColumnType OutputType { get { return NumberType.Float; } }
		private ColumnType OutputType { get { return NumberType.Float; } }


		/// <summary>
		/// Number of thread will be used during training. If unspecified all aviable threads will be use.

Internalize and cleanup recommender project #2451

Internalize and cleanup recommender project #2451

Conversation

Ivanidzo4ka commented Feb 7, 2019 • edited

codecov bot commented Feb 7, 2019 • edited

Codecov Report

Ivanidzo4ka Feb 7, 2019 • edited

Choose a reason for hiding this comment

Ivanidzo4ka Feb 7, 2019 • edited

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin commented Feb 7, 2019 • edited by Ivanidzo4ka

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 7, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 8, 2019

wschin Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin left a comment

Choose a reason for hiding this comment

wschin Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

wschin Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

artidoro Feb 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

artidoro Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

artidoro Feb 8, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 7, 2019 •

edited

codecov bot commented Feb 7, 2019 •

edited

Ivanidzo4ka Feb 7, 2019 •

edited

Ivanidzo4ka Feb 7, 2019 •

edited

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin commented Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 7, 2019 •

edited by Ivanidzo4ka

wschin Feb 8, 2019 •

edited by Ivanidzo4ka

wschin Feb 8, 2019 •

edited by Ivanidzo4ka

wschin Feb 8, 2019 •

edited by Ivanidzo4ka

artidoro Feb 8, 2019 •

edited

artidoro Feb 8, 2019 •

edited by Ivanidzo4ka

artidoro Feb 8, 2019 •

edited by Ivanidzo4ka

artidoro Feb 8, 2019 •

edited by Ivanidzo4ka