Creation of components through MLContext and cleanup (text transform) #2394

abgoswam · 2019-02-03T20:31:30Z

Towards #1798 , #1758

The following transform estimators are being addressed:

TextFeaturizingEstimator

NOTE:

for fixing the text transform API we have to incorporate the API patterns we followed for fixing the learner estimators + the patterns we are following for fixing the transform estimators
This is because the text transform was using a Action delegate (like some of the learner estimators).

The changes are as follows :

Two public extension methods, one for simple arguments and the other for advanced options
Internalize constructors of estimators and transformers
Pass Options objects as arguments instead of Action delegate
Rename Settings to Options
Rename Options objects as options (instead of args or advancedSettings used so far)
Internalize Arguments since the public constructor uses Options. Also a few other fields have been made internal

…form

artidoro · 2019-02-03T21:19:20Z

src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs

@@ -120,7 +120,7 @@ public sealed class Arguments : TransformInputBase
            public TextNormKind VectorNormalizer = TextNormKind.L2;
        }

-        public sealed class Settings
+        public sealed class Options


Can you add a summary tag for each of these settings, and for the Options class? You can take them from the constructor I believe. #Resolved

fixed

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

artidoro · 2019-02-03T21:20:16Z

    public ITransformer Fit(IDataView input)

Can you add a summary tag for Fit? #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:290 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro · 2019-02-03T21:30:42Z

    public static ITransformer Create(IHostEnvironment env, ModelLoadContext ctx)

Could you make this private? #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:465 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro · 2019-02-03T21:30:55Z

    public SchemaShape GetOutputSchema(SchemaShape inputSchema)

Can you add a summary comment? #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:473 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro · 2019-02-03T21:31:18Z

    public const Language DefaultLanguage = Language.English;

Can you add a summary comment, or make this internal?
#Resolved

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:257 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro · 2019-02-03T21:32:17Z

    public sealed class Column : ManyToOneColumn

I think this is only used in the Options class, can you make it internal? #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:62 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro · 2019-02-03T21:32:32Z

src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs

@@ -79,7 +79,7 @@ internal bool TryUnparse(StringBuilder sb)
        /// <summary>
        /// This class exposes <see cref="NgramExtractorTransform"/>/<see cref="NgramHashExtractingTransformer"/> arguments.
        /// </summary>
-        public sealed class Arguments : TransformInputBase
+        internal sealed class Arguments : TransformInputBase


Arguments [](start = 30, length = 9)

Can you rename to Options? #Resolved

If you notice in line #126 below, we have a public Options class, which the public API uses. Hence I kept it as is.

DO u prefer calling it something else, e.g. OptionsInternal ? But it would only be cosmetic, since its 'internal'

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

…form

codecov · 2019-02-04T16:50:05Z

Codecov Report

Merging #2394 into master will decrease coverage by <.01%.
The diff coverage is 60.27%.

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
- Coverage   71.22%   71.22%   -0.01%     
==========================================
  Files         785      785              
  Lines      141030   140978      -52     
  Branches    16116    16113       -3     
==========================================
- Hits       100455   100412      -43     
+ Misses      36106    36095      -11     
- Partials     4469     4471       +2

Flag	Coverage Δ
#Debug	`71.22% <60.27%> (-0.01%)`	⬇️
#production	`67.58% <94.87%> (ø)`	⬆️
#test	`85.28% <20.58%> (-0.03%)`	⬇️

artidoro · 2019-02-05T06:55:32Z

Your PR is also related to #2026. I think you might fix the issue as part of this change.

…form

Ivanidzo4ka · 2019-02-05T19:31:07Z

src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs

@@ -136,10 +136,10 @@ public sealed class Settings
 #pragma warning restore MSML_NoInstanceInitializers // No initializers on instance fields or properties
        }


This whole thing requires proper cleaning/documentation. Do you want to do it in this PR, or you prefer leave it for later cleaning? #Pending

The current PR is to fix the public API

We are doing some cleaning/doc fixes on an opportunistic basis. But we should have a separate issue / PRs for "proper" cleaning and documentation

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

Ivanidzo4ka · 2019-02-05T19:43:00Z

src/Microsoft.ML.Transforms/Text/TextCatalog.cs


        /// <summary>
        /// Transform several text columns into featurized float array that represents counts of ngrams and char-grams.
        /// </summary>
        /// <param name="catalog">The text-related transform's catalog.</param>
        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
        /// <param name="inputColumnNames">Name of the columns to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
-        /// <param name="advancedSettings">Advanced transform settings</param>
+        /// <param name="options">Advanced transform settings.</param>


/// Advanced transform settings. [](start = 7, length = 63)

@sfilipi any chance we have a consensus regarding what to put here?
I can see /// <param name="options">Advanced arguments to the algorithm.</param>
/// <param name="options">Algorithm advanced options.</param>
/// <param name="options">Algorithm advanced settings.</param>
/// <param name="options">Advanced options to the algorithm.</param> #Resolved

Issue #2154 to fix this across codebase (once we finalize what to put in here)

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

Since the object's name is now Options, let;s go with:

/// Advanced options to the algorithm.

if you can search/replace in one of the PR that would be awesome!

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

will do. for now i have fixed this one

In reply to: 254356441 [](ancestors = 254356441,254018114)

…s://github.com/abgoswam/machinelearning into abgoswam/transform_estimator_api_texttransform

abgoswam · 2019-02-05T21:39:42Z

    public ITransformer Fit(IDataView input)

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:290 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

abgoswam · 2019-02-05T21:39:52Z

    public static ITransformer Create(IHostEnvironment env, ModelLoadContext ctx)

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:465 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

abgoswam · 2019-02-05T21:40:03Z

    public SchemaShape GetOutputSchema(SchemaShape inputSchema)

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:473 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

abgoswam · 2019-02-05T21:40:15Z

    public sealed class Column : ManyToOneColumn

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:62 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

abgoswam · 2019-02-05T21:40:24Z

    public const Language DefaultLanguage = Language.English;

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs:257 in c10ca38. [](commit_id = c10ca38, deletion_comment = False)

artidoro

sfilipi · 2019-02-06T16:51:59Z

src/Microsoft.ML.Transforms/Text/TextCatalog.cs


        /// <summary>
        /// Transform several text columns into featurized float array that represents counts of ngrams and char-grams.
        /// </summary>
        /// <param name="catalog">The text-related transform's catalog.</param>
        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
        /// <param name="inputColumnNames">Name of the columns to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
-        /// <param name="advancedSettings">Advanced transform settings</param>
+        /// <param name="options">Advanced transform settings.</param>


settings [](start = 53, length = 8)

Let's not call the Options "settings" :) #Resolved

sfilipi · 2019-02-06T16:53:15Z

src/Microsoft.ML.Transforms/Text/TextCatalog.cs


        /// <summary>
        /// Transform several text columns into featurized float array that represents counts of ngrams and char-grams.
        /// </summary>
        /// <param name="catalog">The text-related transform's catalog.</param>
        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
        /// <param name="inputColumnNames">Name of the columns to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
-        /// <param name="advancedSettings">Advanced transform settings</param>
+        /// <param name="options">Advanced transform settings.</param>
        public static TextFeaturizingEstimator FeaturizeText(this TransformsCatalog.TextTransforms catalog,
            string outputColumnName,
            IEnumerable<string> inputColumnNames,


IEnumerable inputColumnNames [](start = 12, length = 36)

I know it is not your change, but can we go with params, rather than an IEnumerable?

Looking at the sample, having to construct a List is overkill. #Pending

Created issue #2460 to solicit suggestions on proper API for this.

Would not want to block this PR in the meantime.

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

sfilipi · 2019-02-06T16:53:56Z

src/Microsoft.ML.Transforms/Text/TextCatalog.cs


        /// <summary>
        /// Transform several text columns into featurized float array that represents counts of ngrams and char-grams.
        /// </summary>
        /// <param name="catalog">The text-related transform's catalog.</param>
        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
        /// <param name="inputColumnNames">Name of the columns to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
-        /// <param name="advancedSettings">Advanced transform settings</param>
+        /// <param name="options">Advanced transform settings.</param>
        public static TextFeaturizingEstimator FeaturizeText(this TransformsCatalog.TextTransforms catalog,


FeaturizeText [](start = 47, length = 13)

link the sample to this one, since it illustrates the usage of Options. #Resolved

sfilipi · 2019-02-06T16:54:17Z

docs/samples/Microsoft.ML.Samples/Dynamic/TextTransform.cs

-                s.KeepNumbers = false;
-                s.OutputTokens = true;
-                s.TextLanguage = TextFeaturizingEstimator.Language.English; // supports  English, French, German, Dutch, Italian, Spanish, Japanese
+            var customized_pipeline = ml.Transforms.Text.FeaturizeText(customizedColumnName, new List<string> { "SentimentText" }, 


new List { "SentimentText" } [](start = 93, length = 36)

umm :) #Resolved

issue #2460 to discuss this

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

sfilipi · 2019-02-06T16:55:45Z

src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs

+        /// <summary>
+        /// Advanced settings for the <see cref="TextFeaturizingEstimator"/>.
+        /// </summary>
+        public sealed class Options


Options [](start = 28, length = 7)

can you remove this, and use the Arguments above, like for everything else?
The class below is redundant.

make the Factories internal, so they are not visible. #Resolved

I think we don't want to do that.
Main reason why it's bad - is bunch of Factories we don't want to expose.

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

perhaps we should do some of this cleanup outside purview of this PR ...

I need to understand properly how the factories are used etc.

In reply to: 254461580 [](ancestors = 254461580,254358363)

Ivanidzo4ka · 2019-02-07T02:02:13Z

test/Microsoft.ML.Benchmarks/StochasticDualCoordinateAscentClassifierBench.cs

+                    UseWordExtractor = false,
+                }).Fit(loader).Transform(loader);
+
+            var trans = mlContext.Transforms.Text.ExtractWordEmbeddings("Features", "WordEmbeddings_TransformedText", 
                WordEmbeddingsExtractingTransformer.PretrainedModelKind.Sswe).Fit(text).Transform(text);


WordEmbeddingsExtractingTransformer [](start = 16, length = 35)

I though you move it to estimator in your other PR, can you merge with master? #Closed

…s://github.com/abgoswam/machinelearning into abgoswam/transform_estimator_api_texttransform

Ivanidzo4ka · 2019-02-07T18:03:57Z

src/Microsoft.ML.Transforms/Text/TextCatalog.cs

-        /// <example>
-        /// <format type="text/markdown">
-        /// <![CDATA[
-        /// [!code-csharp[FeaturizeText](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/TextTransform.cs)]


TextTransform [](start = 101, length = 13)

you removing this example becaue it doesn't use this exact method? #Resolved

yeap. removed it from here, and added to the API below (this was one of Senja's comments)

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

Ivanidzo4ka · 2019-02-07T18:04:35Z

src/Microsoft.ML.Transforms/Text/TextFeaturizingEstimator.cs

@@ -120,26 +120,59 @@ internal sealed class Arguments : TransformInputBase
            public TextNormKind VectorNormalizer = TextNormKind.L2;
        }

-        public sealed class Settings
+        /// <summary>
+        /// Advanced settings for the <see cref="TextFeaturizingEstimator"/>.


settings [](start = 21, length = 8)

can we call it options? #Resolved

fixed.

am planning to have another PR which fixes this for all the learners/featurizers we converted to Options

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

Ivanidzo4ka

I left two small comments, can you address them?
Otherwise looks good!

text transform

c10ca38

artidoro mentioned this pull request Feb 3, 2019

Creation of components through MLContext: advanced options and other feedback #1798

Closed

Merge branch 'master' into abgoswam/transform_estimator_api_texttrans…

22c9427

…form

artidoro reviewed Feb 3, 2019

View reviewed changes

Merge branch 'master' into abgoswam/transform_estimator_api_texttrans…

7b04a30

…form

Merge branch 'master' into abgoswam/transform_estimator_api_texttrans…

c1b2b67

…form

Ivanidzo4ka reviewed Feb 5, 2019

View reviewed changes

abgoswam added 2 commits February 5, 2019 21:37

review comments

136abd9

Merge branch 'abgoswam/transform_estimator_api_texttransform' of http…

61adaa8

…s://github.com/abgoswam/machinelearning into abgoswam/transform_estimator_api_texttransform

artidoro mentioned this pull request Feb 5, 2019

TextFeaturizingEstimator shouldn't use Argument class as main way of setting itself. #2026

Closed

artidoro approved these changes Feb 5, 2019

View reviewed changes

abgoswam requested a review from sfilipi February 5, 2019 23:55

sfilipi reviewed Feb 6, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 7, 2019

View reviewed changes

abgoswam added 2 commits February 7, 2019 16:46

fix conflicts

1ec8739

Merge branch 'abgoswam/transform_estimator_api_texttransform' of http…

5508581

…s://github.com/abgoswam/machinelearning into abgoswam/transform_estimator_api_texttransform

abgoswam mentioned this pull request Feb 7, 2019

Modify API for FeaturizeText ? #2460

Closed

review comments

47fee69

Ivanidzo4ka reviewed Feb 7, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 7, 2019

View reviewed changes

review comment on options

401fedf

abgoswam merged commit 834e471 into dotnet:master Feb 7, 2019

abgoswam deleted the abgoswam/transform_estimator_api_texttransform branch February 20, 2019 16:58

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

		@@ -136,10 +136,10 @@ public sealed class Settings
		#pragma warning restore MSML_NoInstanceInitializers // No initializers on instance fields or properties
		}

Creation of components through MLContext and cleanup (text transform) #2394

Creation of components through MLContext and cleanup (text transform) #2394

Conversation

abgoswam commented Feb 3, 2019 • edited Loading

artidoro Feb 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

artidoro Feb 3, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2019 • edited Loading

Codecov Report

artidoro commented Feb 5, 2019 • edited Loading

Ivanidzo4ka Feb 5, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 5, 2019 • edited by abgoswam Loading

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

abgoswam commented Feb 5, 2019

abgoswam commented Feb 5, 2019

abgoswam commented Feb 5, 2019

abgoswam commented Feb 5, 2019

abgoswam commented Feb 5, 2019

artidoro left a comment

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 7, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 7, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

abgoswam commented Feb 3, 2019 •

edited

Loading

artidoro Feb 3, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

artidoro Feb 3, 2019 •

edited by abgoswam

Loading

codecov bot commented Feb 4, 2019 •

edited

Loading

artidoro commented Feb 5, 2019 •

edited

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

sfilipi Feb 6, 2019 •

edited by abgoswam

Loading

sfilipi Feb 6, 2019 •

edited by abgoswam

Loading

sfilipi Feb 6, 2019 •

edited by abgoswam

Loading

sfilipi Feb 6, 2019 •

edited by abgoswam

Loading

sfilipi Feb 6, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 7, 2019 •

edited

Loading

Ivanidzo4ka Feb 7, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 7, 2019 •

edited by abgoswam

Loading