Modify API for advanced settings (MatrixFactorizationTrainer) #2204

abgoswam · 2019-01-22T22:23:41Z

Towards #1798 .

This PR addresses the following algos

MatrixFactorizationTrainer

The following changes have been made:

Two public extension methods, one for simple arguments and the other for advanced options
Make constructors internal . Also a few other fields have been made internal
Pass Options objects as arguments instead of Action delegate. Also, added 3 fields to Options for API consistency.
Rename Arguments to Options
Rename Options objects as options (instead of args or advancedSettings used so far)

wschin · 2019-01-23T01:04:24Z

src/Microsoft.ML.Recommender/RecommenderCatalog.cs

+            /// <remarks>
+            /// <para>The basic idea of matrix factorization is finding two low-rank factor marcies to apporimate the training matrix.</para>
+            /// <para>In this module, the expected training data is a list of tuples. Every tuple consists of a column index, a row index,
+            /// and the value at the location specified by the two indexes.


Could you add a reference to Options in the doc string?

Suggested change

/// and the value at the location specified by the two indexes.

/// and the value at the location specified by the two indexes. The training configuration is encoded in <see cref="Options>.

``` #Resolved

added

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

wschin · 2019-01-23T01:07:45Z

test/Microsoft.ML.StaticPipelineTesting/Training.cs

-                .Append(r => (r.label, score: mlContext.Regression.Trainers.MatrixFactorization(r.label, r.matrixRowIndex, r.matrixColumnIndex, onFit: p => pred = p,
-                advancedSettings: args => { args.NumThreads = 1; })));
+                .Append(r => (r.label, score: mlContext.Regression.Trainers.MatrixFactorization(
+                                            r.label, r.matrixRowIndex, r.matrixColumnIndex,


This and the following 2 lines got too many indents? #Resolved

i found this slightly easier to read. did a control+k+d on this file

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

wschin · 2019-01-23T01:08:14Z

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

+                MatrixRowIndexColumnName = itemColumnName,
+                LabelColumnName = labelColumnName,
+                NumIterations = 3,
+                NumThreads = 1,


Please copy my old comment. Thanks. #Resolved

added back

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

wschin · 2019-01-23T01:08:41Z

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

+                MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),
+                LabelColumnName = nameof(MatrixElement.Value),
+                NumIterations = 10,
+                NumThreads = 1,


Same here. Please copy my comment on NumThread. #Resolved

fixed

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

wschin · 2019-01-23T01:09:48Z

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

+                MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),
+                LabelColumnName = nameof(MatrixElement.Value),
+                NumIterations = 100,
+                NumThreads = 1,


May we have my old comment back? :) #Resolved

fixed

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

wschin · 2019-01-23T01:10:08Z

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

+                LabelColumnName = nameof(MatrixElement.Value),
+                LossFunction = MatrixFactorizationTrainer.LossFunctionType.SquareLossOneClass,
+                NumIterations = 100,
+                NumThreads = 1,


Missing my old comment. #Resolved

fixed

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

wschin

Just some minor comments. Overall LGTM.

codecov · 2019-01-24T06:58:06Z

Codecov Report

Merging #2204 into master will not change coverage.
The diff coverage is 95.6%.

@@           Coverage Diff           @@
##           master    #2204   +/-   ##
=======================================
  Coverage   69.78%   69.78%           
=======================================
  Files         786      786           
  Lines      143945   143945           
  Branches    16648    16642    -6     
=======================================
  Hits       100451   100451           
+ Misses      38951    38949    -2     
- Partials     4543     4545    +2

Flag	Coverage Δ
#Debug	`69.78% <95.6%> (ø)`	⬆️
#production	`66.13% <87.09%> (-0.01%)`	⬇️
#test	`84.92% <100%> (ø)`	⬆️

sfilipi · 2019-01-25T06:32:16Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs


        /// <summary>
        /// The <see cref="TrainerInfo"/> contains general parameters for this trainer.
        /// </summary>
        public override TrainerInfo Info { get; }

        /// <summary>
-        /// Legacy constructor initializing a new instance of <see cref="MatrixFactorizationTrainer"/> through the legacy
-        /// <see cref="Arguments"/> class.
+        /// Constructor initializing a new instance of <see cref="MatrixFactorizationTrainer"/> through the <see cref="Options"/> class.


Constructor initializing [](start = 12, length = 24)

replace with "Initializes".

That seems to be standard for .net docs for ctors. #Resolved

sfilipi · 2019-01-25T06:32:48Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

-        /// <param name="args">An instance of the legacy <see cref="Arguments"/> to apply advanced parameters to the algorithm.</param>
-        private MatrixFactorizationTrainer(IHostEnvironment env, Arguments args) : base(env, LoadNameValue)
+        /// <param name="options">An instance of <see cref="Options"/> to apply advanced parameters to the algorithm.</param>
+        internal MatrixFactorizationTrainer(IHostEnvironment env, Options options) : base(env, LoadNameValue)


internal [](start = 8, length = 8)

think @Ivanidzo4ka asked for those marked with [BestFriend] #Resolved

sfilipi · 2019-01-25T06:46:24Z

src/Microsoft.ML.Recommender/RecommenderCatalog.cs

+            /// </para>
+            /// </remarks>
+            /// <param name="options">Advanced arguments to the algorithm.</param>
+            public MatrixFactorizationTrainer MatrixFactorization(


public MatrixFactorizationTrainer MatrixFactorization [](start = 9, length = 56)

please link to the Matrix factorization sample.

sfilipi

MatrixFactorizationTrainer API

d80bf0c

abgoswam requested review from wschin and sfilipi January 22, 2019 22:23

wschin reviewed Jan 23, 2019

View reviewed changes

wschin approved these changes Jan 23, 2019

View reviewed changes

abgoswam added 2 commits January 23, 2019 18:36

review comments

7a3e223

fix typos

3f5a952

codemzs closed this Jan 24, 2019

codemzs reopened this Jan 24, 2019

sfilipi reviewed Jan 25, 2019

View reviewed changes

sfilipi approved these changes Jan 25, 2019

View reviewed changes

abgoswam added 3 commits January 25, 2019 17:25

review comments

1d7911f

fix incorrect reference

e60facc

Merge branch 'master' into abgoswam/action_arguments_mf

f3b157d

abgoswam merged commit 2497833 into dotnet:master Jan 26, 2019

abgoswam deleted the abgoswam/action_arguments_mf branch February 20, 2019 16:59

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify API for advanced settings (MatrixFactorizationTrainer) #2204

Modify API for advanced settings (MatrixFactorizationTrainer) #2204

abgoswam commented Jan 22, 2019 •

edited

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin Jan 23, 2019 •

edited by abgoswam

abgoswam Jan 23, 2019

wschin left a comment

codecov bot commented Jan 24, 2019 •

edited

sfilipi Jan 25, 2019 •

edited by abgoswam

sfilipi Jan 25, 2019 •

edited by abgoswam

sfilipi Jan 25, 2019

sfilipi left a comment

	/// and the value at the location specified by the two indexes.
	/// and the value at the location specified by the two indexes. The training configuration is encoded in <see cref="Options>.
	``` #Resolved

Modify API for advanced settings (MatrixFactorizationTrainer) #2204

Modify API for advanced settings (MatrixFactorizationTrainer) #2204

Conversation

abgoswam commented Jan 22, 2019 • edited

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin Jan 23, 2019 • edited by abgoswam

Choose a reason for hiding this comment

abgoswam Jan 23, 2019

Choose a reason for hiding this comment

wschin left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 24, 2019 • edited

Codecov Report

sfilipi Jan 25, 2019 • edited by abgoswam

Choose a reason for hiding this comment

sfilipi Jan 25, 2019 • edited by abgoswam

Choose a reason for hiding this comment

sfilipi Jan 25, 2019

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

abgoswam commented Jan 22, 2019 •

edited

wschin Jan 23, 2019 •

edited by abgoswam

wschin Jan 23, 2019 •

edited by abgoswam

wschin Jan 23, 2019 •

edited by abgoswam

wschin Jan 23, 2019 •

edited by abgoswam

wschin Jan 23, 2019 •

edited by abgoswam

wschin Jan 23, 2019 •

edited by abgoswam

codecov bot commented Jan 24, 2019 •

edited

sfilipi Jan 25, 2019 •

edited by abgoswam

sfilipi Jan 25, 2019 •

edited by abgoswam