Convert TextNormalizer to estimator #1276

Ivanidzo4ka · 2018-10-17T06:00:55Z

Convert TextNormalizer to estimator

artidoro · 2018-10-17T17:25:58Z

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


            [Argument(ArgumentType.AtMostOnce, HelpText = "Whether to keep numbers or remove them.", ShortName = "num", SortOrder = 2)]
-            public bool KeepNumbers = true;
+            public bool KeepNumbers = TextNormalizerEstimator.Defaults.KeepNumbers;
        }

        internal const string Summary = "A text normalization transform that allows normalizing text case, removing diacritical marks, punctuation marks and/or numbers." +
            " The transform operates on text input as well as vector of tokens/text (vector of ReadOnlyMemory).";

        public const string LoaderSignature = "TextNormalizerTransform";


TextNormalizerTransform [](start = 47, length = 23)

We could get rid of this and use nameof(TextNormalizerTransform) instead? otherwise we shoudl probably make it private or internal #Resolved

artidoro · 2018-10-17T17:27:49Z

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

-            // Greek letters combined with diacritics:
-            "ΆΑ", "ΈΕ", "ΉΗ", "ΊΙ", "ΌΟ", "ΎΥ", "ΏΩ", "ΐι", "ΪΙ", "ΫΥ", "άα", "έε", "ήη", "ίι", "ΰυ", "ϊι",
-            "ϋυ", "όο", "ύυ", "ώω", "ϓϒ", "ϔϒ",
+        public TextNormalizerTransform(IHostEnvironment env, ColumnInfo[] columns) :


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

Can we make this internal, since users will construct the transform only through the estimators? #Resolved

Actually no, non-trainable transformers should have public ctors.

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

Ok good to know!

In reply to: 226071035 [](ancestors = 226071035,226024916)

artidoro · 2018-10-17T17:34:44Z

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

+            None = 2
+        }
+
+        public static class Defaults


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

Could this be internal, or private instead of public? The user should not need it. #Resolved

artidoro · 2018-10-17T17:38:53Z

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

-                // Hebrew combining diacritics
-                ch >= 0x0591 && ch <= 0x05BD || ch == 0x05C1 || ch == 0x05C2 || ch == 0x05C4 ||
-                ch == 0x05C5 || ch == 0x05C7 ||
+        private TextNormalizerEstimator(IHostEnvironment env, TextNormalizerTransform transformer)


TextNormalizerEstimator [](start = 16, length = 23)

When do we use this constructor? The above TextNormalizerEstimator(IHostEnvironment env, params TextNormalizerTransform.ColumnInfo[] columns) can call directly into base. #Resolved

in public constructor above

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

How about:

public TextNormalizerEstimator(IHostEnvironment env, params TextNormalizerTransform.ColumnInfo[] columns)
:base(Contracts.CheckRef(env, nameof(env)).Register(nameof(TextNormalizerEstimator)), new TextNormalizerTransform(env, columns))
{
}

And we get rid of the private one?

In reply to: 226032246 [](ancestors = 226032246,226028712)

artidoro · 2018-10-17T17:48:06Z

test/Microsoft.ML.Tests/Transformers/TextNormalizer.cs

+        }
+
+        [Fact]
+        public void TestOldSavingAndLoading()


TestOldSavingAndLoading [](start = 20, length = 23)

Both saving and loading seem to be done with the new transform code. Since you changed the versioninfo, it might be useful to have an other test where you load a model that was saved using the old transform code, so that we check that those models can still be loaded? #Resolved

Not anymore

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

change defaults visibility as well

Ivanidzo4ka · 2018-10-17T18:18:02Z

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

        private static VersionInfo GetVersionInfo()
        {
            return new VersionInfo(
                modelSignature: "TEXTNORM",
-                verWrittenCur: 0x00010001, // Initial
+                //verWrittenCur: 0x00010001, // Initial
+                verWrittenCur: 0x00010002, // Params for each column.


verWrittenCur: 0x00010002, // Params for each column. [](start = 16, length = 53)

@Zruty0, I'm actually not sure how benefitial and usefull to provide options for each column rather than provide set of columns and options for all of them. Would be nice if you can comment. #Closed

This would unblock certain pigsty optimizations, where you can bundle multiple calls to text normalizer (with different params) into one estimator. But since this is not incurring a data pass, I'm ok with not having a per-column option.

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

Zruty0 · 2018-10-17T19:49:50Z

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

-            return h.Apply("Loading Model", ch => new TextNormalizerTransform(h, ctx, input));
+            var type = inputSchema.GetColumnType(srcCol);
+            if (!TextNormalizerEstimator.IsColumnTypeValid(type))
+                throw Host.ExceptParam(nameof(inputSchema), TextNormalizerEstimator.ExpectedColumnType);


ExceptParam [](start = 27, length = 11)

ExceptSchemaMismatch #Resolved

Zruty0 · 2018-10-17T19:51:01Z

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

+                _columns = new ColumnInfo[columnsLength];
+                for (int i = 0; i < columnsLength; i++)
+                {
+                    // *** Binary format ***


/ *** Binary format *** [](start = 21, length = 23)

yea I don't think it's necessary to introduce that extra code path just to keep per-column options. #Resolved

Zruty0 · 2018-10-17T19:53:19Z

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

-                    for (int i = 0; i < src.Count; i++)
+                    inputSchema.TryGetColumnIndex(_parent._columns[i].Input, out int srcCol);
+                    var srcType = inputSchema.GetColumnType(srcCol);
+                    _types[i] = srcType.IsVector ? new VectorType(TextType.Instance) : srcType;


VectorType(TextType.Instance) : srcType [](start = 55, length = 39)

I believe you should validate srcType here.
Also, are you really creating a variable vector of text? Why do you even change type at all?

Oh, I see, it's dropping the empty ones. Wow

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

The validation comment still stands

In reply to: 226072884 [](ancestors = 226072884,226072116)

What kind of validation?

In reply to: 226072949 [](ancestors = 226072949,226072884,226072116)

Zruty0 · 2018-10-17T19:54:19Z

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


-            if (src.IsEmpty)
+                input.Schema.TryGetColumnIndex(_parent._columns[iinfo].Input, out int srcCol);
+                var srcType = input.Schema.GetColumnType(srcCol);


srcType [](start = 20, length = 7)

var srcType = input.Schema[_parent._columns[iinfo].Input].Type #Resolved

Zruty0 · 2018-10-17T19:56:44Z

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

+
+        public static bool IsColumnTypeValid(ColumnType type) => (type.ItemType.IsText);
+
+        internal const string ExpectedColumnType = "Expected Text item type";


Expected Text item type [](start = 52, length = 23)

how about 'Text or vector of text' ? #Resolved

Zruty0 · 2018-10-17T19:57:35Z

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

+                if (!inputSchema.TryFindColumn(colInfo.Input, out var col))
+                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.Input);
+                if (!IsColumnTypeValid(col.ItemType))
+                    throw Host.ExceptParam(nameof(inputSchema), ExpectedColumnType);


ExceptParam [](start = 31, length = 11)

ExceptSchemaMismatch #Resolved

Zruty0 · 2018-10-17T19:59:55Z

test/Microsoft.ML.Tests/Transformers/TextNormalizer.cs

+        }
+
+        [Fact]
+        public void TextNormalizeStatic()


TextNormalizeStatic [](start = 20, length = 19)

please don't bundle pigsty and non-pigsty tests in one class, put it next to other pigsty tests #Resolved

Zruty0

Ivanidzo4ka · 2018-10-18T01:22:29Z

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

                {
                    var col = new StopWordsCol();
-                    col.Source = wordTokCols[i];
+col.Source = wordTokCols[i];


Source [](start = 4, length = 6)

fix layout

artidoro

Convert TextNormalizer to estimator

50af436

Ivanidzo4ka added the API Issues pertaining the friendly API label Oct 17, 2018

Ivanidzo4ka requested review from Zruty0, artidoro and singlis October 17, 2018 06:01

Ivan Matantsev added 2 commits October 17, 2018 09:57

make it test!

85cf744

make it build

7a01ed7

artidoro reviewed Oct 17, 2018

View reviewed changes

address comments.

6574974

change defaults visibility as well

Ivanidzo4ka commented Oct 17, 2018

View reviewed changes

small update

6d14bf4

Zruty0 reviewed Oct 17, 2018

View reviewed changes

Zruty0 approved these changes Oct 17, 2018

View reviewed changes

Ivan Matantsev added 4 commits October 17, 2018 15:32

revert multicolumn params

66abcfb

fix build, update tests.

e0a822b

SchemaMismatch

a160b9b

remove extra lines

381523a

Ivanidzo4ka commented Oct 18, 2018

View reviewed changes

artidoro approved these changes Oct 18, 2018

View reviewed changes

Ivanidzo4ka merged commit a285f8d into dotnet:master Oct 18, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert TextNormalizer to estimator #1276

Convert TextNormalizer to estimator #1276

Ivanidzo4ka commented Oct 17, 2018

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018

artidoro Oct 17, 2018

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Oct 17, 2018

artidoro Oct 17, 2018

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Oct 17, 2018

Ivanidzo4ka Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018

Zruty0 Oct 17, 2018

Ivanidzo4ka Oct 17, 2018

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 left a comment

Ivanidzo4ka Oct 18, 2018

artidoro left a comment


		public static bool IsColumnTypeValid(ColumnType type) => (type.ItemType.IsText);

		internal const string ExpectedColumnType = "Expected Text item type";

Convert TextNormalizer to estimator #1276

Convert TextNormalizer to estimator #1276

Conversation

Ivanidzo4ka commented Oct 17, 2018

artidoro Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

artidoro Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

artidoro Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited 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

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

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

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

artidoro Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Oct 17, 2018 •

edited by Ivanidzo4ka

Loading