Creation of components through MLContext and cleanup (text related transforms) #2393

abgoswam · 2019-02-03T18:27:04Z

Towards #1798 , #1758, #1760

The following transform estimators are being addressed:

LatentDirichletAllocationEstimator
WordEmbeddingsExtractingEstimator
TokenizingByCharactersEstimator
WordTokenizingEstimator
WordBagEstimator
WordHashBagEstimator
NgramExtractingEstimator
NgramHashingEstimator
StopWordsRemovingEstimator
CustomStopWordsRemovingEstimator
TextNormalizingEstimator

The changes are as follows :

Internalize constructors of estimators and transformers Creation of components through MLContext: advanced options and other feedback #1798
Rename Arguments -> Options Creation of components through MLContext: advanced options and other feedback #1798
Internalize Options when they are not used by public constructor. Arguments class should be made internal when possible #1758
Rename Options objects as options (instead of args or advancedSettings used so far) Creation of components through MLContext: advanced options and other feedback #1798
Move ColumnInfo to the estimators The ColumnInfo structure should live in the estimators, rather than transformers #1760

codecov · 2019-02-03T19:03:55Z

Codecov Report

Merging #2393 into master will increase coverage by <.01%.
The diff coverage is 87.37%.

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
+ Coverage   71.26%   71.26%   +<.01%     
==========================================
  Files         785      785              
  Lines      140946   140939       -7     
  Branches    16108    16108              
==========================================
  Hits       100440   100440              
+ Misses      36039    36031       -8     
- Partials     4467     4468       +1

Flag	Coverage Δ
#Debug	`71.26% <87.37%> (ø)`	⬆️
#production	`67.61% <87.52%> (ø)`	⬆️
#test	`85.32% <84.61%> (ø)`	⬆️

artidoro · 2019-02-03T21:10:38Z

    public sealed class Column : OneToOneColumn

can you make this internal as well?
#Resolved

Refers to: src/Microsoft.ML.Transforms/Text/LdaTransform.cs:109 in 92e6f2d. [](commit_id = 92e6f2d, deletion_comment = False)

artidoro · 2019-02-03T21:11:39Z

    public sealed class LdaSummary

If possible this one too should become internal. I am not sure where it is used though, so might need to double check. #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/LdaTransform.cs:167 in 92e6f2d. [](commit_id = 92e6f2d, deletion_comment = False)

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

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

        {
            Contracts.CheckValue(env, nameof(env));
            _host = env.Register(nameof(LatentDirichletAllocationEstimator));
            _columns = columns.ToImmutableArray();
        }

+        public sealed class ColumnInfo
+        {
+            public readonly string Name;


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

Could you add a summary tag for each one of these public entries?

And could you add a summary for the ColumnInfo object? #Resolved

artidoro · 2019-02-03T21:14:43Z

    public LatentDirichletAllocationTransformer Fit(IDataView input)

Could you add a summary for this method? #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/LdaTransform.cs:1172 in 92e6f2d. [](commit_id = 92e6f2d, deletion_comment = False)

…malizingEstimator

abgoswam · 2019-02-05T18:51:03Z

    public sealed class LdaSummary

this one is currently used by Static API via the OnFit() delegate, to provide details about the topics discovered by LightLDA.

making it internal would break the Static API

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

Refers to: src/Microsoft.ML.Transforms/Text/LdaTransform.cs:167 in 92e6f2d. [](commit_id = 92e6f2d, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T20:16:42Z

    /// </summary>

nit: I would end summary here, and wrap next paragraph into <remark>

Refers to: src/Microsoft.ML.Transforms/Text/TextCatalog.cs:483 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T20:18:56Z

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

        /// Returns the <see cref="SchemaShape"/> of the schema which will be produced by the transformer.
        /// Used for schema propagation and verification in a pipeline.
        /// </summary>``` #Closed

---
Refers to: src/Microsoft.ML.Transforms/Text/TokenizingByCharacters.cs:587 in a81c3d2. [](commit_id = a81c3d23707b61fd97d685e36d6f929deceffb13, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T20:19:17Z

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

@@ -94,7 +94,7 @@ internal bool TryUnparse(StringBuilder sb)
        /// </summary>
        public sealed class TokenizeColumn : OneToOneColumn { }


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

also internal #Resolved

The class is internal, so it should be fine

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

deleted actually. had 0 references :)

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

Ivanidzo4ka · 2019-02-05T20:19:42Z

internal #Closed

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:41 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T20:20:07Z

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

@@ -220,7 +220,7 @@ internal bool TryUnparse(StringBuilder sb)

        /// <summary>
        /// This class is a merger of <see cref="ValueToKeyMappingTransformer.Options"/> and
-        /// <see cref="NgramExtractingTransformer.Arguments"/>, with the allLength option removed.
+        /// <see cref="NgramExtractingTransformer.Options"/>, with the allLength option removed.
        /// </summary>
        public abstract class ArgumentsBase


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

internal? #Resolved

fixed

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

Ivanidzo4ka · 2019-02-05T20:21:08Z

    public static IDataTransform Create(IHostEnvironment env, NgramExtractorArguments extractorArgs, IDataView input,

can you merge with master, I think I tackle it recently. #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:369 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T20:22:27Z

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

+            public readonly string Name;
+            public readonly string InputColumnName;
+
+            public ColumnInfo(string name, string inputColumnName = null)


summary to constructor. #Resolved

fixed

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

Ivanidzo4ka · 2019-02-05T20:22:54Z

    public class Column : OneToOneColumn

internal #Resolved

Refers to: src/Microsoft.ML.Transforms/Text/WordTokenizing.cs:43 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

artidoro · 2019-02-05T23:01:28Z

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

+                InputColumnName = inputColumnName ?? name;
+                Separators = separators ?? new[] { ' ' };
+            }
+        }

        public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
        {


///

/// Returns the of the schema which will be produced by the transformer.
/// Used for schema propagation and verification in a pipeline.
///
#Resolved

fixed

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

abgoswam · 2019-02-05T23:16:36Z

    public sealed class NgramExtractorArguments : ArgumentsBase, INgramExtractorFactoryFactory

I will create a separate issue to scrub all the entrypoint APIs and fix their namings. If you see they derive from ArgumentsBase so need to consider if they should have suffix *Options

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

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:249 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:31:47Z

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TokenizingByCharacters.cs:587 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

abgoswam · 2019-02-05T23:31:58Z

fixed i think ?

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

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:41 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

abgoswam · 2019-02-05T23:32:10Z

    public static IDataTransform Create(IHostEnvironment env, NgramExtractorArguments extractorArgs, IDataView input,

fixed. looks like u had missed it. ;)

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

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:369 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

abgoswam · 2019-02-05T23:32:24Z

    public class Column : OneToOneColumn

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/WordTokenizing.cs:43 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

abgoswam · 2019-02-05T23:32:33Z

    public void Dispose()

implementing IDisposable, so cannot make this internal.

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

Refers to: src/Microsoft.ML.Transforms/Text/LdaTransform.cs:684 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:01Z

    public static bool IsColumnTypeValid(ColumnType type) =>

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/StopWordsRemovingTransformer.cs:551 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:09Z

    public static bool IsColumnTypeValid(ColumnType type) => (type.GetItemType() is TextType);

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextNormalizing.cs:453 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:16Z

    public static bool IsColumnTypeValid(ColumnType type) => type.GetItemType() is TextType;

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TokenizingByCharacters.cs:557 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:25Z

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/TextNormalizing.cs:501 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:33Z

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/StopWordsRemovingTransformer.cs:1077 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:40Z

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/StopWordsRemovingTransformer.cs:587 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:47Z

    public static VersionInfo GetVersionInfo()

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs:79 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:33:57Z

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/WordEmbeddingsExtractor.cs:635 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:34:05Z

    public abstract class ArgumentsBase : TransformInputBase

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/WordTokenizing.cs:67 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

abgoswam · 2019-02-05T23:34:19Z

fixed

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

Refers to: src/Microsoft.ML.Transforms/Text/WordTokenizing.cs:409 in aae84eb. [](commit_id = aae84eb, deletion_comment = False)

Ivanidzo4ka · 2019-02-05T23:39:58Z

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

@@ -42,7 +42,7 @@ internal sealed class ExtractorColumn : ManyToOneColumn

    internal static class WordBagBuildingTransformer
    {
-        public sealed class Column : ManyToOneColumn
+        internal sealed class Column : ManyToOneColumn


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

it's already part of internal class, you no longer need to make it subclass internal :)

artidoro

Ivanidzo4ka · 2019-02-05T23:48:56Z

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

+            /// </summary>
+            public readonly string Name;
+            /// <summary>
+            /// Name of column to transform. If set to <see langword="null"/>, the value of the <cref see="Name"/> will be used as source.


/// Name of column to transform. If set to , the value of the will be used as source. [](start = 11, length = 127)

as Zeeshan A pointed in other PRs, you need to left only first sentence. It's always populated with value. (Only if it's property in ColumnInfo)
I would suggest to sweep whole PR. #Resolved

not quite sure what u mean .

scrub whole PR for what ?

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

If you add anywhere else that comment to InputColumnName inside ColumnInfo, you need to change it.
If you didn't add it in other places, you just need to fix this one.

In reply to: 254094414 [](ancestors = 254094414,254093004)

So it should just say

/// <summary> /// Name of column to transform. /// </summary>

?

In reply to: 254094755 [](ancestors = 254094755,254094414,254093004)

@artidoro what do you put in them? Sorry, it fills like billions of same PRs, and it's hard to find for me, but I know what Artidoro address that in his PR recently.

In reply to: 254096126 [](ancestors = 254096126,254094755,254094414,254093004)

Yes that's how I fixed it! #Resolved

Ivanidzo4ka · 2019-02-05T23:59:46Z

    public static IDataTransform Create(IHostEnvironment env, NgramExtractorArguments extractorArgs, IDataView input,

Well, it's part of internal class now, so I decide it's not worth time to change methods modifiers :)

In reply to: 460846657 [](ancestors = 460846657,460788737)

Refers to: src/Microsoft.ML.Transforms/Text/WordBagTransform.cs:369 in a81c3d2. [](commit_id = a81c3d2, deletion_comment = False)

Ivanidzo4ka · 2019-02-06T00:18:09Z

test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamples.cs

@@ -473,7 +473,7 @@ private void TextFeaturizationOn(string dataPath)
                    BagOfTrichar: r.Message.TokenizeIntoCharacters().ToNgrams(ngramLength: 3, weighting: NgramExtractingEstimator.WeightingCriteria.TfIdf),

                    // NLP pipeline 4: word embeddings.
-                    Embeddings: r.Message.NormalizeText().TokenizeText().WordEmbeddings(WordEmbeddingsExtractingTransformer.PretrainedModelKind.GloVeTwitter25D)
+                    Embeddings: r.Message.NormalizeText().TokenizeText().WordEmbeddings(WordEmbeddingsExtractingEstimator.PretrainedModelKind.GloVeTwitter25D)


NormalizeText [](start = 42, length = 13)

If you update any of cookbooks you also need to update https://github.com/dotnet/machinelearning/blob/master/docs/code/MlNetCookBook.md

I wish we had way to autogenerate it somehow... #Resolved

Ivanidzo4ka

lda

92e6f2d

abgoswam requested a review from artidoro February 3, 2019 18:27

artidoro mentioned this pull request Feb 3, 2019

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

Closed

abgoswam changed the title ~~Creation of components through MLContext and cleanup (Text Transforms)~~ Creation of components through MLContext and cleanup (several Text related transforms) Feb 3, 2019

abgoswam changed the title ~~Creation of components through MLContext and cleanup (several Text related transforms)~~ Creation of components through MLContext and cleanup (text related transforms) Feb 3, 2019

artidoro reviewed Feb 3, 2019

View reviewed changes

abgoswam added 8 commits February 4, 2019 16:22

Merge branch 'master' into abgoswam/transform_estimator_api_lda

cb4b573

WordEmbeddingsExtractingEstimator

e4543fe

TokenizingByCharactersEstimator

02ee301

WordTokenizingEstimator

ed579f4

WordBagEstimator, WordHashBagEstimator

60e83a7

NgramExtractingEstimator, NgramHashingEstimator

4991506

StopWordsRemovingEstimator; CustomStopWordsRemovingEstimator; TextNor…

42cb783

…malizingEstimator

making Column internal

c23c464

review comments regarding adding summary comments

a81c3d2

Ivanidzo4ka reviewed Feb 5, 2019

View reviewed changes

sfilipi mentioned this pull request Feb 5, 2019

SampleDatasetUtils is in disarray #2420

Closed

merge with latest master; fix merge conflicts

aae84eb

artidoro reviewed Feb 5, 2019

View reviewed changes

fix merge conflicts; took care of review comments

c8207dd

Ivanidzo4ka reviewed Feb 5, 2019

View reviewed changes

artidoro approved these changes Feb 5, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 5, 2019

View reviewed changes

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

fix merge conflicts

aa892e5

Ivanidzo4ka reviewed Feb 6, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 6, 2019

View reviewed changes

fixed CookBook + summary comment on InputColumnName

7df1ed0

abgoswam merged commit 753f158 into dotnet:master Feb 6, 2019

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

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

		@@ -94,7 +94,7 @@ internal bool TryUnparse(StringBuilder sb)
		/// </summary>
		public sealed class TokenizeColumn : OneToOneColumn { }

Creation of components through MLContext and cleanup (text related transforms) #2393

Creation of components through MLContext and cleanup (text related transforms) #2393

Conversation

abgoswam commented Feb 3, 2019 • edited Loading

codecov bot commented Feb 3, 2019 • edited Loading

Codecov Report

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

artidoro commented Feb 3, 2019 • edited by abgoswam Loading

abgoswam commented Feb 5, 2019

Ivanidzo4ka commented Feb 5, 2019 • edited Loading

Ivanidzo4ka 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

Choose a reason for hiding this comment

Ivanidzo4ka 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 commented Feb 5, 2019 • edited by abgoswam Loading

Ivanidzo4ka Feb 5, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 5, 2019 • edited by abgoswam Loading

artidoro Feb 5, 2019 • edited by abgoswam Loading

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

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

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

abgoswam commented Feb 5, 2019

Ivanidzo4ka Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

artidoro left a 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

Choose a reason for hiding this comment

artidoro Feb 6, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 5, 2019

Ivanidzo4ka Feb 6, 2019 • edited by abgoswam Loading

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

codecov bot commented Feb 3, 2019 •

edited

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

artidoro commented Feb 3, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 5, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 5, 2019 •

edited

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 5, 2019 •

edited

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 5, 2019 •

edited by abgoswam

Loading

artidoro Feb 5, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 5, 2019 •

edited

Loading

Ivanidzo4ka Feb 5, 2019 •

edited by abgoswam

Loading

artidoro Feb 6, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 6, 2019 •

edited by abgoswam

Loading