adding some trainer extensions on the StandardLearners catalog. Correcting namespace, and names #1682

sfilipi · 2018-11-20T08:31:18Z

The last PR addressing #1318.
Closes #1318

…cting namespace, and names.

Ivanidzo4ka · 2018-11-20T17:50:49Z

I see lot of tests failing with different issues, can you take a look on them? #Closed

Zruty0 · 2018-11-20T18:08:39Z

src/Microsoft.ML.Recommender/RecommenderCatalog.cs

+        /// <param name="labelColumn">The name of the label column.</param>
+        /// <param name="advancedSettings">A delegate to apply all the advanced arguments to the algorithm.</param>
+        /// <param name="context">The <see cref="TrainerEstimatorContext"/> for additional input data to training.</param>
+        public static MatrixFactorizationTrainer MatrixFactorization(this RegressionContext.RegressionTrainers ctx,


RegressionContext [](start = 74, length = 17)

I don't like this to be part of regression context. #Resolved

Zruty0 · 2018-11-20T18:10:01Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

+        WhiteningKind kind = VectorWhiteningTransformer.Defaults.Kind,
+        float eps = VectorWhiteningTransformer.Defaults.Eps,
+        int maxRows = VectorWhiteningTransformer.Defaults.MaxRows,
+        int pcaNum = VectorWhiteningTransformer.Defaults.PcaNum)


fix indentation #Resolved

Zruty0

…per classes. Addressing dotnet#1685

sfilipi · 2018-11-21T17:05:20Z

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs

@@ -249,7 +250,7 @@ private string GetBuildPrefix()
 #endif
        }

-        [Fact(Skip = "Execute this test if you want to regenerate the core_manifest and core_ep_list files")]
+        [Fact]


[Fact] [](start = 5, length = 9)

revert #Resolved

…o swap names. Adding the Recommendation context.

Ivanidzo4ka · 2018-11-21T18:28:02Z

src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs

+
+        /// <summary>
+        /// Initializes a new instance of <see cref="ValueToKeyMappingEstimator"/>.
+        /// </summary>


I think we want to have proper comment with explanation what this extension for other than this. #Closed

Ivanidzo4ka · 2018-11-21T18:28:12Z

src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs

+
+        /// <summary>
+        /// Initializes a new instance of <see cref="ValueToKeyMappingEstimator"/> loading the terms to use from <paramref name="file"/>.
+        /// </summary>


same as above. #Closed

Ivanidzo4ka · 2018-11-21T18:29:58Z

src/Microsoft.ML.HalLearners/ProjectionCatalog.cs

-
-namespace Microsoft.ML
-{
-    public static class ProjectionCatalog


out of curiosity, why? #Closed

Thy said "one per assembly" so moved it to the HalLearners catalog.

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

Ivanidzo4ka · 2018-11-21T18:30:52Z

src/Microsoft.ML.PCA/PcaTransform.cs

@@ -691,6 +692,10 @@ internal static class Defaults
        {
        }

+        /// <summary>Initializes a new instance of <see cref="PrincipalComponentAnalysisEstimator"/>.</summary>


///
Initializes a new instance of .
[](start = 8, length = 103)

ugh, formating. #Closed

Ivanidzo4ka · 2018-11-21T18:31:42Z

src/Microsoft.ML.Recommender/RecommenderCatalog.cs

+
+            /// <summary>
+            /// Initializing a new instance of <see cref="MatrixFactorizationTrainer"/>.
+            /// </summary>


can we have better comment for user facing api? #Closed

Ivanidzo4ka · 2018-11-21T18:32:30Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+
+        /// <summary>
+        /// Initializes a new instance of <see cref="MultiClassNaiveBayesTrainer"/>
+        /// </summary>


Can we have slightly better comments for user facing api? #Closed

Ivanidzo4ka · 2018-11-21T18:33:37Z

src/Microsoft.ML.TensorFlow/TensorflowCatalog.cs

+        /// </summary>
+        /// <param name="catalog">The transform's catalog.</param>
+        /// <param name="args">The <see cref="TensorFlowTransform.Arguments"/> specifying the inputs and the settings of the <see cref="TensorFlowEstimator"/>.</param>
+        /// <param name="tensorFlowModel"></param>


/// [](start = 8, length = 42)

/// The pre-trained TensorFlow model. #Closed

Ivanidzo4ka · 2018-11-21T18:35:07Z

src/Microsoft.ML.Transforms/ConversionsCatalog.cs

+        /// <returns></returns>
+        public static KeyToBinaryVectorMappingEstimator MapKeyToBinaryVector(this TransformsCatalog.ConversionTransforms catalog,
+            params KeyToBinaryVectorMappingTransformer.ColumnInfo[] columns)
+        => new KeyToBinaryVectorMappingEstimator(CatalogUtils.GetEnvironment(catalog), columns);


=> new [](start = 8, length = 6)

That difference in => position here and in other method drives my OCD nuts. #Closed

Ivanidzo4ka · 2018-11-21T18:35:13Z

src/Microsoft.ML.Transforms/ConversionsCatalog.cs

+        /// </summary>
+        /// <param name="catalog">The categorical transform's catalog.</param>
+        /// <param name="columns">The input column.</param>
+        /// <returns></returns>


/// [](start = 8, length = 23)

omit #Closed

Ivanidzo4ka · 2018-11-21T18:35:21Z

src/Microsoft.ML.Transforms/ConversionsCatalog.cs

+        /// <param name="catalog">The categorical transform's catalog.</param>
+        /// <param name="inputColumn">The name of the input column of the transformation.</param>
+        /// <param name="outputColumn">The name of the column produced by the transformation.</param>
+        /// <returns></returns>


/// [](start = 8, length = 23)

omit #Closed

Ivanidzo4ka · 2018-11-21T18:35:57Z

src/Microsoft.ML.Transforms/ExtensionsCatalog.cs


-    public static class MissingValueReplacerCatalog
-    {
        /// <summary>
        /// Initializes a new instance of <see cref="MissingValueReplacingEstimator"/>
        /// </summary>


we need better summary for user facing api. #Closed

Ivanidzo4ka · 2018-11-21T18:36:54Z

    /// <param name="catalog">The categorical transform's catalog.</param>

this description look odd across all methods in this file. Why it's categorical transform's catalog. #Closed

Refers to: src/Microsoft.ML.Transforms/ExtensionsCatalog.cs:17 in 1c44ea2. [](commit_id = 1c44ea2, deletion_comment = False)

Ivanidzo4ka · 2018-11-21T18:38:16Z

src/Microsoft.ML.Transforms/TextCatalog.cs

-        /// </summary>
-        /// <param name="catalog">The transform's catalog.</param>
-        /// <param name="columns"> Describes the parameters of LDA for each column pair.</param>
-        public static LatentDirichletAllocationEstimator LatentDirichletAllocation(this TransformsCatalog.TextTransforms catalog, params LatentDirichletAllocationTransformer.ColumnInfo[] columns)


LatentDirichletAllocationEstimator [](start = 22, length = 34)

I think Abhishek moved LDA to separate file yesterday, can you merge and wipe it as well? #Closed

this is post Abishek's merge. By the way, do we want LDA in text or projections?

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

Sorry, got confused between catalog and static extensions. LDA doesn't make much sense outside of text, we use terminology as words, topics, etc. so Text.

In reply to: 235501427 [](ancestors = 235501427,235500231)

Ivanidzo4ka · 2018-11-21T18:42:12Z

.Append(mlContext.Transforms.Categorical.MapValueToKey("Label"), TransformerScope.TrainTest)

Conversion

Just reflect changes in CookbookSamplesDynamicApi.cs #Closed

Refers to: docs/code/MlNetCookBook.md:1300 in 1c44ea2. [](commit_id = 1c44ea2, deletion_comment = False)

artidoro · 2018-11-21T18:44:57Z

Can you rename the file? #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/ColumnsCopying.cs:4 in 1c44ea2. [](commit_id = 1c44ea2, deletion_comment = False)

Ivanidzo4ka · 2018-11-21T18:45:05Z

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

@@ -62,7 +63,7 @@ public void MatrixFactorizationSimpleTrainAndPredict()
            var data = reader.Read(new MultiFileSource(GetDataPath(TestDatasets.trivialMatrixFactorization.trainFilename)));

            // Create a pipeline with a single operator.
-            var pipeline = new MatrixFactorizationTrainer(mlContext, userColumnName, itemColumnName, labelColumnName,
+            var pipeline = mlContext.Recommendation().Trainers.MatrixFactorization(userColumnName, itemColumnName, labelColumnName,


mlContext.Recommendation().Trainers.MatrixFactorization( [](start = 27, length = 56)

why I can do
mlContext.BinaryClassification.Trainers.FastTree
but for recommendaion i need to do
mlContext.Recommendation().Trainers
I'm talking about braces. #Closed

Because RecommendationContext lives in a different assembly and nuget. So it is an extension method to MLContext, rather than a property.

there are no recommendation trainers in ml.net core nuget, no point on having it there and empty. Time series will be the same.

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

artidoro · 2018-11-21T18:49:41Z

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

@@ -16,7 +15,7 @@ public static class ImageEstimatorsCatalog
        /// </summary>
        /// <param name="catalog">The transform's catalog.</param>
        /// <param name="columns">The name of the columns containing the image paths(first item of the tuple), and the name of the resulting output column (second item of the tuple).</param>
-        public static ImageGrayscalingEstimator Grayscale(this TransformsCatalog catalog, params (string input, string output)[] columns)
+        public static ImageGrayscalingEstimator ConvertToGrayscale(this TransformsCatalog catalog, params (string input, string output)[] columns)


ConvertToGrayscale [](start = 48, length = 18)

Why are we changing the name here? Are we not using the same name as the estimator?

The names of the methods are usually PerformAction. ConvertToGrayscale" felt more informative than "Grayscale"

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

I mean why are we not using ImageGrayScalingEstimator? These are calling the dynamic API, so I don't see why we should change names.

In reply to: 235504239 [](ancestors = 235504239,235503615)

There is a broader discussion for it here: #1690

I do see your point. As @CESARDELATORRE pointed out, the most correct thing to do would be to call those CreateActionPerformingEstimator.

We chose to go with just the ActionPerforming verb.

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members#names-of-methods

In reply to: 235504834 [](ancestors = 235504834,235504239,235503615)

Ivanidzo4ka · 2018-11-26T22:17:57Z

src/Microsoft.ML.Transforms/ExtensionsCatalog.cs

        /// </summary>
-        /// <param name="catalog">The categorical transform's catalog.</param>
+        /// <param name="catalog">The transform extensions' catalog.</param>


' [](start = 58, length = 1)

sneaky apostrophe is sneaky.
across whole file. #Closed

This is fine, no? The extensions are transform extensions; there are many of them. Possession on plural is followed by apos.

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

TIL

In reply to: 236465988 [](ancestors = 236465988,236449673)

Ivanidzo4ka · 2018-11-26T22:20:15Z

var estimator = mlContext.Transforms.Categorical.MapValueToKey("Label")

This one as well

Refers to: docs/code/MlNetCookBook.md:264 in 437e367. [](commit_id = 437e367, deletion_comment = False)

Ivanidzo4ka · 2018-11-26T22:20:28Z

.Append(mlContext.Transforms.Categorical.MapValueToKey("Label"), TransformerScope.TrainTest)

And this one?

Refers to: docs/code/MlNetCookBook.md:642 in 437e367. [](commit_id = 437e367, deletion_comment = False)

Ivanidzo4ka

Ivanidzo4ka · 2018-11-27T00:31:18Z

var estimator = mlContext.Transforms.Categorical.MapValueToKey("Label")

-Doctor, everyone ignores me, please tell me what to do?
-Next one, please!

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

Refers to: docs/code/MlNetCookBook.md:264 in 437e367. [](commit_id = 437e367, deletion_comment = False)

adding some trainer extensions on the StandartLearners catalog. Corre…

ea47333

…cting namespace, and names.

sfilipi self-assigned this Nov 20, 2018

sfilipi added the API Issues pertaining the friendly API label Nov 20, 2018

sfilipi added this to the 1118 milestone Nov 20, 2018

Zruty0 reviewed Nov 20, 2018

View reviewed changes

Zruty0 approved these changes Nov 20, 2018

View reviewed changes

sfilipi added 2 commits November 20, 2018 13:09

adding extensions for Tensorflow and for other transforms in the Wrap…

d7142c5

…per classes. Addressing dotnet#1685

regenerate the C# api

4fc4678

sfilipi changed the title ~~adding some trainer extensions on the StandartLearners catalog. Correcting namespace, and names~~ [WIP] adding some trainer extensions on the StandartLearners catalog. Correcting namespace, and names Nov 20, 2018

sfilipi added 5 commits November 20, 2018 21:32

Merge branch 'master' into 1318epilogue

b4fc529

merging from master

bd8008e

post merge edits

9463539

Fixing the weight init in Linear SVM

e729276

correct the order of arguments

1f4b01d

sfilipi changed the title ~~[WIP] adding some trainer extensions on the StandartLearners catalog. Correcting namespace, and names~~ adding some trainer extensions on the StandardLearners catalog. Correcting namespace, and names Nov 21, 2018

sfilipi commented Nov 21, 2018

View reviewed changes

renaming the NgramContingEstimator and the NgramExtractingEstimator t…

1c44ea2

…o swap names. Adding the Recommendation context.

Ivanidzo4ka reviewed Nov 21, 2018

View reviewed changes

artidoro reviewed Nov 21, 2018

View reviewed changes

Addressing the PR comments.

437e367

Ivanidzo4ka reviewed Nov 26, 2018

View reviewed changes

Ivanidzo4ka approved these changes Nov 26, 2018

View reviewed changes

sfilipi added 2 commits November 26, 2018 16:08

post merge fixes

53c8a8e

another merge

e00f545

sfilipi added 6 commits November 26, 2018 22:09

removing the pipeline sweeper added during merge.

4a33d59

Csharp Api fixes

e7f081c

pull remote

4c13aad

regenerating after a clean build

d8f255b

Merge branch 'master' into 1318epilogue

c048734

merge and fox, yet again.

cbc9c4f

sfilipi merged commit 481c377 into dotnet:master Nov 27, 2018

sfilipi deleted the 1318epilogue branch November 27, 2018 20:30

sfilipi mentioned this pull request Dec 7, 2018

MLContext extensions for trainers #1319

Closed

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

adding some trainer extensions on the StandardLearners catalog. Correcting namespace, and names #1682

adding some trainer extensions on the StandardLearners catalog. Correcting namespace, and names #1682

Conversation

sfilipi commented Nov 20, 2018 • edited Loading

Ivanidzo4ka commented Nov 20, 2018 • edited Loading

Zruty0 Nov 20, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 Nov 20, 2018 • edited by sfilipi Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

sfilipi Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Nov 21, 2018 • edited Loading

Ivanidzo4ka Nov 21, 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 commented Nov 21, 2018 • edited Loading

artidoro commented Nov 21, 2018 • edited by sfilipi Loading

Ivanidzo4ka Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Nov 26, 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 commented Nov 26, 2018

Ivanidzo4ka commented Nov 26, 2018

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Nov 27, 2018

sfilipi commented Nov 20, 2018 •

edited

Loading

Ivanidzo4ka commented Nov 20, 2018 •

edited

Loading

Zruty0 Nov 20, 2018 •

edited by sfilipi

Loading

Zruty0 Nov 20, 2018 •

edited by sfilipi

Loading

sfilipi Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka commented Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

Ivanidzo4ka commented Nov 21, 2018 •

edited

Loading

artidoro commented Nov 21, 2018 •

edited by sfilipi

Loading

Ivanidzo4ka Nov 21, 2018 •

edited

Loading

artidoro Nov 21, 2018 •

edited

Loading

artidoro Nov 21, 2018 •

edited

Loading

Ivanidzo4ka Nov 26, 2018 •

edited

Loading