Changed default NGrams for FeaturizerText from 1 to 2

[michaelgsharp]

This fixes #4749. All the other changes in that issue have been done, this was the only one that was left, which will now allow us to close this issue.

@justinormont can you just verify that is the correct NGramLength to change? Based on the issue, and the other linked issue, this is what I believe is the correct spot. The standard error does go down in the tests with this, which is the expected behavior based on the linked issues.

[harishsk]

The issue #4749 is a tracking issue for many changes. Once this PR is merged, github is going to automatically close that issue because of the text "fixes #..." in the description above.
Can you please remove that text or add some other word after "fixes" so that this bug isn't automatically closed?

We can close this issue after all the issues that are tracked by #4749 are fixed.

[michaelgsharp]

Hey Harish, all the other issues in there have been fixed already, I was planning on changing them all in this PR, but going through them all they were already changed. I am not sure why the other PR's didn't reference that issue. Let me see if I can find the other PR's and link them to this one.

[michaelgsharp]

Actually, let me double check. There are several places that defaults are used, let me make sure I changed them all. If I missed some I will add those changes to this PR as well.

[justinormont]

Bigrams are in the output now, which is good. For example, Word.generally|accepted. Tri-charactergrams are present.

The change from unigrams to bigrams appears to be working.

[codecov]

Codecov Report

Merging #5243 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5243   +/-   ##
=======================================
  Coverage   73.46%   73.47%           
=======================================
  Files        1010     1010           
  Lines      187974   187998   +24     
  Branches    20261    20263    +2     
=======================================
+ Hits       138103   138131   +28     
- Misses      44384    44385    +1     
+ Partials     5487     5482    -5     

Flag	Coverage Δ
#Debug	73.47% <100.00%> (+<0.01%)	⬆️
#production	69.29% <100.00%> (+<0.01%)	⬆️
#test	87.38% <100.00%> (+<0.01%)	⬆️

Impacted Files	Coverage Δ
...soft.ML.Transforms/Text/WrappedTextTransformers.cs	99.10% <100.00%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs	97.45% <100.00%> (ø)
...osoft.ML.Tests/Transformers/TextFeaturizerTests.cs	99.64% <100.00%> (ø)
...rc/Microsoft.ML.Featurizers/DateTimeTransformer.cs	87.15% <0.00%> (-1.81%)	⬇️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs	85.15% <0.00%> (-1.75%)	⬇️
src/Microsoft.ML.Maml/MAML.cs	24.75% <0.00%> (-1.46%)	⬇️
...Microsoft.ML.Transforms/OptionalColumnTransform.cs	76.87% <0.00%> (-0.87%)	⬇️
...soft.ML.Data/DataView/DataViewConstructionUtils.cs	85.51% <0.00%> (+0.02%)	⬆️
test/Microsoft.ML.Functional.Tests/ModelFiles.cs	96.38% <0.00%> (+0.12%)	⬆️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	89.45% <0.00%> (+0.15%)	⬆️
... and 6 more

[justinormont]

Two smaller PRs will be easier to review.

Together, this is modifying 105 files by changing both the default ngram length, and AveragedPerceptron's default iterations.

For instance, the AveragedPerceptron change should only affect baseline outputs where AveragedPerceptron is used (including in ensembles). But I see some changes in SVM files, which shouldn't change due to the AP change, but might have a TextTransform in the pipeline. It is just hard to disentangle.

[michaelgsharp]

Yeah, I am going to be breaking this up. We seem to have a memory issue with the NGram stuff, so I will be splitting these out to get these in while I work on that.

You said the SVM files shoudln't change, but might have a TextTransform in the pipeline. If they do have a text transform should they change? Or should we only see changes where AveragedPerceptron is used and no where else? Or, is this change a bad thing for svm as well?

[justinormont]

Expected output changes

This PR is changing both AveragedPerceptron and the text transform. If the pipeline has either of these components, I would expect the pipeline's output to change.

Any pipeline with neither AveragedPerceptron nor the text transform, should have the same output as before. Modifying the other trainers' iteration counts is not benchmarked and, by default, I would assume will not be beneficial.

Memory footprint

The memory footprint will increase and is absolutely worth it for the accuracy gains of our default pipeline. The pipeline being implemented in this PR, Bigrams+tricharactergrams w/ AveragedPerceptron{iter=10}, is the default in TLC and ML․NET AutoML for text and is highly tested/benchmarked. It's the result of around 50-150k runs on the combinatorics of a variety of datasets; primarily testing for ngram lengths, trainers, and hyperparameters. This pipeline was found to be an optimal default. Other featurization options help on specific datasets (see: list).

The memory increase will occur in a few places, (1) in the text transform's dictionary where it stores all seen ngrams, (2) in the iDataView cache if used, (3) in tree trainers at the dataset transpose step for row-wise to column-wise, and (4) in high multi-class linear trainers which store the weight for each feature times each class.

In the disentangled PR handing the ngram change, we should dig into any memory issues which arise. It may be a limitation of our build/testing platform.

Perhaps in the future we should write a doc on tips for low memory model training and serving.

[michaelgsharp]

Thanks for the detailed responses @justinormont.

I will check the files that were changed again and make sure that they all have either AveragedPerceptron or a text transform. If I find any that don't I will change the location where I am overwriting that value.

I think I have the memory increase issue figured out, but I am still looking into it. Yes, the memory should go up, but it was exploding way faster then it should. A 70 Kb file (250 lines) was exploding into 1.7 GB per Ngram length during training. So when the NGram length was 2, it was using 3.4 GB for that small file. It appears to be because we have implemented it so it creates all the possible memory upfront, whether it is being used or not. I have changed that and the memory usage is much smaller now. Still verifying that my changes had no impact on any other tests though.

[justinormont]

I wouldn't anticipate this file being modified due to changing AveragedPerceptron's default iterations from 1 to 10. Is there a TextTransform in this pipeline causing the change?

[justinormont]

Modifying within OnlineLinear.cs seems to be affecting multiple trainers besides AveragedPerceptron. You may need to find a way to target just AveragedPerceptron.

[michaelgsharp]

I can do that. Question though, would it be bad to update it for them all? Or is just that we know for AveragedPerceptron 10 is better, but thats not true for the others?

[justinormont]

For the other trainers, it's unknown if 10 iterations will be better or worse. For most trainers, NumberOfIterations/NumberOfTrees and LearningRate are the primary hyperparameters; changing them has a large impact.

Since the existing defaults in the other trainers are backed by some level of benchmarking when they were created, I would keep with the existing value. Moving from 1 to 10 iterations, we know the runtime will increase and we should demonstrate the value (accuracy gain) of this extra time spent.

For AveragedPerceptron, we did benchmarks showing the gain in having 10 iterations as the new default.

Subset of the AveragedPerceptron benchmarks are in the original issue -- #2305.

[michaelgsharp]

Closing this PR as #5248, #5258, #5290 fix this issue.