Exposed ngram extraction options in TextFeaturizer by zeahmed · Pull Request #2911 · dotnet/machinelearning

zeahmed · 2019-03-11T20:06:50Z

This PR fixes #2802, fixes #2838 and partially addresses #838,.

codecov · 2019-03-11T20:57:28Z

Codecov Report

Merging #2911 into master will increase coverage by 0.36%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #2911      +/-   ##
==========================================
+ Coverage   71.82%   72.19%   +0.36%     
==========================================
  Files         812      796      -16     
  Lines      142719   142020     -699     
  Branches    16092    16044      -48     
==========================================
+ Hits       102513   102526      +13     
+ Misses      35827    35115     -712     
  Partials     4379     4379

Flag	Coverage Δ
#Debug	`72.19% <94.73%> (+0.36%)`	⬆️
#production	`67.97% <94.23%> (ø)`	⬆️
#test	`88.31% <100%> (+2.09%)`	⬆️

Impacted Files	Coverage Δ
...crosoft.ML.Transforms/EntryPoints/TextAnalytics.cs	`41.25% <ø> (ø)`	⬆️
...rc/Microsoft.ML.Transforms/Text/TextNormalizing.cs	`97.11% <ø> (ø)`	⬆️
...icrosoft.ML.Functional.Tests/DataTransformation.cs	`100% <100%> (ø)`	⬆️
...soft.ML.Transforms/Text/WrappedTextTransformers.cs	`95.28% <100%> (+0.38%)`	⬆️
...crosoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs	`98.08% <100%> (-0.01%)`	⬇️
...oft.ML.Transforms/Text/TextFeaturizingEstimator.cs	`88.7% <93.18%> (-0.31%)`	⬇️
src/Microsoft.ML.Maml/MAML.cs	`24.75% <0%> (-1.46%)`	⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`84.7% <0%> (-0.21%)`	⬇️
...icrosoft.ML.Mkl.Components/MklComponentsCatalog.cs	`94.28% <0%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`100% <0%> (ø)`	⬆️
... and 18 more

Ivanidzo4ka · 2019-03-11T21:01:11Z

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

+            /// The maximum number of grams to store in the dictionary, for each level of ngrams,
+            /// from 1 (in position 0) up to ngramLength (in position ngramLength-1)
+            /// </summary>
+            public int[] MaxNumTerms;


MaxNumTerms [](start = 25, length = 11)

@wschin calls it maximumNgramsCount would be nice to be consistent. #Resolved

Ivanidzo4ka · 2019-03-11T21:02:16Z

        // TODO #2802: Update FeaturizeText to allow specifications of word-grams and char-grams.

Can be removed? #Resolved

Refers to: test/Microsoft.ML.Functional.Tests/DataTransformation.cs:138 in d3e2841. [](commit_id = d3e2841, deletion_comment = False)

Ivanidzo4ka · 2019-03-11T21:13:47Z

        public bool UsePredefinedStopWordRemover = false;

So originally we had Factory for StopWordRemover, which I think I change to this boolean field.
Would be nice to bring that stuff back, but it's out of scope of this PR.
Should I create separate issue or you think you can do it in this PR? #Resolved

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

Ivanidzo4ka · 2019-03-11T21:15:15Z

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

+            internal Column Columns;

            [Argument(ArgumentType.AtMostOnce, HelpText = "Dataset language or 'AutoDetect' to detect language per row.", ShortName = "lang", SortOrder = 3)]
            public Language Language = DefaultLanguage;


Language [](start = 28, length = 8)

Not related to your PR, but whole point of this thing is to set or autodetect language, we left autodetect outside of ML.NET so it's looks completely unnecessary here, but as I said in other comment, this is out of scope of this PR. #Resolved

Yes, let use this issue for tracking this #838.

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

Ivanidzo4ka

ganik · 2019-03-11T23:05:04Z

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

+            public Options()
+            {
+                WordFeatureExtractor = new WordBagEstimator.Options();
+                CharFeatureExtractor = new WordBagEstimator.Options() { NgramLength = 3, AllLengths = false };


Shouldn't these fields initialized to null by default? And then fallback to WordFeatureExtractorFactory / CharFeatureExtractorFactory whenever you try to use them and they are not defined. This way code flow from entrypoint / maml will be same as with API. #Resolved

WordFeatureExtractor and CharFeatureExtractor both can be null. When they are null its means do not apply the respective transforms. So, there is no way to detect if null was set by user or its a default value.

Right now, WordFeatureExtractorFactory / CharFeatureExtractorFactory is preferred if the maml is used.

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

ganik · 2019-03-11T23:06:20Z

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

-            [Argument(ArgumentType.Multiple, HelpText = "Ngram feature extractor to use for characters (WordBag/WordHashBag).", ShortName = "charExtractor", NullName = "<None>", SortOrder = 12)]
-            public INgramExtractorFactoryFactory CharFeatureExtractor = new NgramExtractorTransform.NgramExtractorArguments() { NgramLength = 3, AllLengths = false };
+            [Argument(ArgumentType.Multiple, Name = "CharFeatureExtractor", HelpText = "Ngram feature extractor to use for characters (WordBag/WordHashBag).", ShortName = "charExtractor", NullName = "<None>", SortOrder = 12)]
+            internal INgramExtractorFactoryFactory CharFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments() { NgramLength = 3, AllLengths = false };


INgramExtractorFactoryFactory [](start = 21, length = 29)

FactoryFactory :), is it possible just have name INgramExtractorFactory
#Resolved

Good point but we already have a class name INgramExtractorFactory

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

Line 435 in ce0c917

internal interface INgramExtractorFactory

and there is a relation between INgramExtractorFactory and INgramExtractorFactoryFactory. I leave it for now. Maybe you can create an issue to track it if you think this needs to be renamed?

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

ganik · 2019-03-11T23:06:39Z

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

-            [Argument(ArgumentType.Multiple, HelpText = "Ngram feature extractor to use for words (WordBag/WordHashBag).", ShortName = "wordExtractor", NullName = "<None>", SortOrder = 11)]
-            public INgramExtractorFactoryFactory WordFeatureExtractor = new NgramExtractorTransform.NgramExtractorArguments();
+            [Argument(ArgumentType.Multiple, Name = "WordFeatureExtractor", HelpText = "Ngram feature extractor to use for words (WordBag/WordHashBag).", ShortName = "wordExtractor", NullName = "<None>", SortOrder = 11)]
+            internal INgramExtractorFactoryFactory WordFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments();


INgramExtractorFactoryFactory [](start = 21, length = 29)

same here FactoryFactory :) #Resolved

zeahmed · 2019-03-11T23:22:36Z

        public bool UsePredefinedStopWordRemover = false;

Yes, let use this issue for tracking this #838.

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

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

wschin · 2019-03-12T17:27:56Z

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

+            internal INgramExtractorFactoryFactory WordFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments();
+
+            public WordBagEstimator.Options WordFeatureExtractor;



Public field needs document. In addition, to internalize IFactory please follow the pattern Tom and I have built in this PR --- PR description of #2851 #Resolved

wschin · 2019-03-12T17:28:30Z

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

+            internal INgramExtractorFactoryFactory CharFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments() { NgramLength = 3, AllLengths = false };
+
+            public WordBagEstimator.Options CharFeatureExtractor;



Public field needs document. In addition, to internalize IFactory please follow the pattern Tom and I have built in this PR --- PR description of #2851 #Resolved

wschin · 2019-03-12T17:38:17Z

I don't know how CharFeatureExtractor can affect this region because there is no change. Can you make user's CharFeatureExtractor can finally affect view? #Resolved

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

wschin · 2019-03-12T22:16:52Z

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

+
+            public Options()
+            {
+                WordFeatureExtractor = new WordBagEstimator.Options();


WordFeatureExtractor [](start = 16, length = 20)

This will kill the initial value of WordFeatureExtractorFactory. How about remove new NgramExtractorTransform.NgramExtractorArguments() in internal INgramExtractorFactoryFactory WordFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments();? A similar comment applies to CharFeatureExtractor.
#Resolved

zeahmed added 4 commits March 11, 2019 12:47

Exposed ngram options in TextFeaturizer.

d42d923

Resolved merged conflicts.

4f9bab8

Merge remote-tracking branch 'upstream/master' into config_textfeat

88d6604

Resolved merged conflicts.

d3e2841

zeahmed requested review from Ivanidzo4ka, rogancarr and wschin March 11, 2019 20:06

Ivanidzo4ka reviewed Mar 11, 2019

View reviewed changes

Ivanidzo4ka approved these changes Mar 11, 2019

View reviewed changes

zeahmed added 2 commits March 11, 2019 14:36

Merge remote-tracking branch 'upstream/master' into config_textfeat

2e66dce

Merge remote-tracking branch 'upstream/master' into config_textfeat

f2ecc95

ganik reviewed Mar 11, 2019

View reviewed changes

Zruty0 mentioned this pull request Mar 11, 2019

Make TextFeaturizer's extractors etc. configurable again #838

Closed

zeahmed added 2 commits March 11, 2019 16:35

Addressed reviewers' comments.

7c5b324

Resolved merged conflicts.

e03104b

wschin reviewed Mar 12, 2019

View reviewed changes

zeahmed added 2 commits March 12, 2019 11:30

Addressed reviewers' comments.

d569492

Resolved merged conflicts.

6d5e7ac

wschin reviewed Mar 12, 2019

View reviewed changes

wschin approved these changes Mar 12, 2019

View reviewed changes

Addressed reviewers' comments.

e14ac38

zeahmed merged commit ec6ff08 into dotnet:master Mar 13, 2019

singlis mentioned this pull request Mar 18, 2019

I want to convert a text classification sample from ML 0.4 to ML 0.11 and cannot find equivalent code for calling Transforms.Text.FeaturizeText #2994

Closed

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

		internal INgramExtractorFactoryFactory WordFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments();

		public WordBagEstimator.Options WordFeatureExtractor;

		internal INgramExtractorFactoryFactory CharFeatureExtractorFactory = new NgramExtractorTransform.NgramExtractorArguments() { NgramLength = 3, AllLengths = false };

		public WordBagEstimator.Options CharFeatureExtractor;

Conversation

zeahmed commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Ivanidzo4ka Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka commented Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka commented Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

ganik Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

ganik Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganik Mar 11, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed commented Mar 11, 2019

Uh oh!

wschin Mar 12, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Mar 12, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin commented Mar 12, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wschin Mar 12, 2019 • edited by zeahmed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zeahmed commented Mar 11, 2019 •

edited

Loading

codecov bot commented Mar 11, 2019 •

edited

Loading

Ivanidzo4ka Mar 11, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Mar 11, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka commented Mar 11, 2019 •

edited by zeahmed

Loading

Ivanidzo4ka Mar 11, 2019 •

edited by zeahmed

Loading

ganik Mar 11, 2019 •

edited by zeahmed

Loading

ganik Mar 11, 2019 •

edited by zeahmed

Loading

zeahmed Mar 11, 2019 •

edited

Loading

ganik Mar 11, 2019 •

edited by zeahmed

Loading

wschin Mar 12, 2019 •

edited by zeahmed

Loading

wschin Mar 12, 2019 •

edited by zeahmed

Loading

wschin commented Mar 12, 2019 •

edited by zeahmed

Loading

wschin Mar 12, 2019 •

edited by zeahmed

Loading