Skip to content

Fixed the TextTransform bug where chargrams where being computed differently when using with/without word tokenizer. #548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zeahmed merged 8 commits into from
Jul 26, 2018
Merged

Fixed the TextTransform bug where chargrams where being computed differently when using with/without word tokenizer. #548

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
868a72f
Fixed the TextTransform bug where chargrams where being computed diff…
zeahmed Jul 17, 2018
f482eea
Merge remote-tracking branch 'upstream/master' into texttransform_bug
zeahmed Jul 19, 2018
64390ee
Addressed reviewers' comments.
zeahmed Jul 19, 2018
2201433
Addressed reviewers' comments.
zeahmed Jul 21, 2018
d1dd161
Addressed reviewers' comments.
zeahmed Jul 25, 2018
f458cdb
Merge remote-tracking branch 'upstream/master' into texttransform_bug
zeahmed Jul 25, 2018
90cdf2a
Fixed bugs detected by code analyzer.
zeahmed Jul 25, 2018
3ed6e87
Addressed reviewers' comments.
zeahmed Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 69 additions & 4 deletions src/Microsoft.ML.Transforms/Text/CharTokenizeTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,13 @@ public sealed class Arguments : TransformInputBase
public const string LoaderSignature = "CharToken";
public const string UserName = "Character Tokenizer Transform";

public static uint CurrentModelVersion = 0x00010002;

@TomFinley TomFinley Jul 21, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CurrentModelVersion [](start = 27, length = 19)

Hmmm. Just to clarify something, because having this be a constant is essential for the correctness of your serialization and deserialization routines, turning it into a public static mutable field is probably not a good idea.

I question the worth of making this variable * at all*, but even if you should please don't make this public, and this should definitely be a const. Also "current" version is not descriptive. It is the version where you changed the behavior of the separating characters to be the single character <US> vs having start and end characters. If you really want a variable, it should be something like VariableSeparatorIsUnitSeparator or somesuch to actually describe what version 0x00010002 is. But again I'd question how much worth such a const as this would provide. #Resolved

private static VersionInfo GetVersionInfo()
{
return new VersionInfo(
modelSignature: "CHARTOKN",
verWrittenCur: 0x00010001, // Initial

@TomFinley TomFinley Jul 21, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verWrittenCur: 0x00010001, // Initial [](start = 16, length = 37)

See other examples. You will see that we keep commented out the version strings from old versions, to track them and so we know why we bumped the version each time. This is important: we have live code in our deserializers that is meant to handle those version bumps, and for that reason we like to have documentation on what exactly changed, so that when we have tests on this or that version in our deserializers, we know why they changed. #Resolved

verReadableCur: 0x00010001,
verWrittenCur: CurrentModelVersion,

@TomFinley TomFinley Jul 21, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verWrittenCur: CurrentModelVersion, [](start = 16, length = 35)

Please refer to other components where we bumped the version number. (If I search for 0x00010002 I see 43 matching files, so you should be able to find examples easily.) You will see that in those other versions we have a comment that describes why the version was changed. This is blank. #Resolved

verReadableCur: CurrentModelVersion,
verWeCanReadBack: 0x00010001,
loaderSignature: LoaderSignature);
}
Expand All @@ -84,6 +85,7 @@ private static VersionInfo GetVersionInfo()
private volatile string _keyValuesStr;
private volatile int[] _keyValuesBoundaries;

private const ushort UnitSeparator = 0x1f;
private const ushort TextStartMarker = 0x02;
private const ushort TextEndMarker = 0x03;
private const int TextMarkersCount = 2;
Expand Down Expand Up @@ -119,6 +121,7 @@ private CharTokenizeTransform(IHost host, ModelLoadContext ctx, IDataView input)
// <base>
// byte: _useMarkerChars value.
_useMarkerChars = ctx.Reader.ReadBoolByte();
CurrentModelVersion = ctx.Header.ModelVerReadable;

@TomFinley TomFinley Jul 21, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CurrentModelVersion [](start = 12, length = 19)

Oh I see what you've done here.. this is not good, static variables are essentially global state. They are not particular to an instance. #Resolved


_type = GetOutputColumnType();
SetMetadata();
Expand Down Expand Up @@ -397,10 +400,11 @@ private ValueGetter<VBuffer<ushort>> MakeGetterVec(IRow input, int iinfo)
int cv = Infos[iinfo].TypeSrc.VectorSize;
Contracts.Assert(cv >= 0);

var version = GetVersionInfo();
var getSrc = GetSrcGetter<VBuffer<DvText>>(input, iinfo);
var src = default(VBuffer<DvText>);
return
(ref VBuffer<ushort> dst) =>

ValueGetter<VBuffer<ushort>> valueGetterOldVersion = (ref VBuffer<ushort> dst) =>
{
getSrc(ref src);

Expand Down Expand Up @@ -438,6 +442,67 @@ private ValueGetter<VBuffer<ushort>> MakeGetterVec(IRow input, int iinfo)

dst = new VBuffer<ushort>(len, values, dst.Indices);
};

ValueGetter < VBuffer<ushort> > valueGetterCurrentVersion = (ref VBuffer<ushort> dst) =>
{
getSrc(ref src);

int len = 0;

for (int i = 0; i < src.Count; i++)
{
if (src.Values[i].HasChars)
{
len += src.Values[i].Length;

if (i > 0)
len += 1; // add UnitSeparator character to len that will be added
}
}

if (_useMarkerChars)
len += TextMarkersCount;

var values = dst.Values;
if (len > 0)
{
if (Utils.Size(values) < len)
values = new ushort[len];

int index = 0;

// VBuffer<DvText> can be a result of either concatenating text columns together
// or application of word tokenizer before char tokenizer in TextTransform.
//
// Considering VBuffer<DvText> as a single text stream.
// Therefore, prepend and append start and end markers only once i.e. at the start and at end of vector.
// Insert UnitSeparator after every piece of text in the vector.
if (_useMarkerChars)
values[index++] = TextStartMarker;

for (int i = 0; i < src.Count; i++)
{
if (!src.Values[i].HasChars)
continue;

if (i > 0)
values[index++] = UnitSeparator;

for (int ich = 0; ich < src.Values[i].Length; ich++)
{
values[index++] = src.Values[i][ich];
}
}

if (_useMarkerChars)
values[index++] = TextEndMarker;

Contracts.Assert(index == len);
}

dst = new VBuffer<ushort>(len, values, dst.Indices);
};
return CurrentModelVersion < version.VerReadableCur ? valueGetterOldVersion : valueGetterCurrentVersion;

@zeahmed zeahmed Jul 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomFinley and @Ivanidzo4ka, Is this correct way to maintain backward compatibility? #Resolved

@TomFinley TomFinley Jul 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. For one thing you've stored this in a static variable, which is not correct, since this behavior will be specific to the instance. So if you read two instances of the transform, one with the old version, one with the new version, you'd break one or the other (depending on the order in which you read them). As a general rule, not just in ML.NET but in any coding anywhere, storing instance specific data inside a static variable is generally a bad idea. :)

For the sake of resaving an old model to the new format, you ought to store a boolean variable in the new version of the file, to control whether it is the old or new version. So the saver will have ctx.Writer.WriteBoolByte(_isStartEnd) and in the reader have something like _isStartEnd = ctx.Header.ModelVerReadable < 0x00010002 || ctx.Reader.ReadBoolByte(). This hypothetical _isStartEnd will be private readonly, and emphatically not public static.

If anything about that is not clear let me know, thanks @zeahmed!

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

}
}
}
54 changes: 25 additions & 29 deletions src/Microsoft.ML.Transforms/Text/TextTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,30 @@ public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataV
view = new ConcatTransform(h, new ConcatTransform.Arguments() { Column = xfCols }, view);
}

if (tparams.NeedsNormalizeTransform)

@zeahmed zeahmed Jul 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinormont, I moved TextNormalizer above WordTokenizer. Let me know if there is any adverse effect of it? #Resolved

@justinormont justinormont Jul 20, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there are averse effects of running TextNormalizer before WordTokenizer. I expect it is different, but assume not adversely so. For instance (depending on the specifics of the tokenizer), I don't can be split into {I, do, n't} but if removing punctuation first, it may be split to {I, dont}.

If you make me a build, I'll run a manual regression test. Unfortunately we don't currently have running nightly regression tests to tell us if the text datasets decreased in their core metrics.

#Resolved

@TomFinley TomFinley Jul 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change. It's far from clear to me that if someone asks to remove stopwords that they meant for the content of stopwords to be retained in the chargrams. At the very least this ought to be a configurable option.

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

@zeahmed zeahmed Jul 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomFinley, Can you elaborate more on this? Your point is not clear to me. Don't you see it good to have TextNormalizer applied before WordTokenizer?

In reply to: 204201054 [](ancestors = 204201054,204000786)

@TomFinley TomFinley Jul 25, 2018 •
edited by zeahmed
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry thought you were saying something else for some reason got confused. #Resolved

{
var xfCols = new TextNormalizerCol[textCols.Length];
string[] dstCols = new string[textCols.Length];
for (int i = 0; i < textCols.Length; i++)
{
dstCols[i] = GenerateColumnName(view.Schema, textCols[i], "TextNormalizer");
tempCols.Add(dstCols[i]);
xfCols[i] = new TextNormalizerCol() { Source = textCols[i], Name = dstCols[i] };
}

view = new TextNormalizerTransform(h,
new TextNormalizerArgs()
{
Column = xfCols,
KeepDiacritics = tparams.KeepDiacritics,
KeepNumbers = tparams.KeepNumbers,
KeepPunctuations = tparams.KeepPunctuations,
TextCase = tparams.TextCase
}, view);

textCols = dstCols;
}

if (tparams.NeedsWordTokenizationTransform)
{
var xfCols = new DelimitedTokenizeTransform.Column[textCols.Length];
Expand All @@ -281,34 +305,6 @@ public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataV
view = new DelimitedTokenizeTransform(h, new DelimitedTokenizeTransform.Arguments() { Column = xfCols }, view);
}

if (tparams.NeedsNormalizeTransform)
{
string[] srcCols = wordTokCols == null ? textCols : wordTokCols;
var xfCols = new TextNormalizerCol[srcCols.Length];
string[] dstCols = new string[srcCols.Length];
for (int i = 0; i < srcCols.Length; i++)
{
dstCols[i] = GenerateColumnName(view.Schema, srcCols[i], "TextNormalizer");
tempCols.Add(dstCols[i]);
xfCols[i] = new TextNormalizerCol() { Source = srcCols[i], Name = dstCols[i] };
}

view = new TextNormalizerTransform(h,
new TextNormalizerArgs()
{
Column = xfCols,
KeepDiacritics = tparams.KeepDiacritics,
KeepNumbers = tparams.KeepNumbers,
KeepPunctuations = tparams.KeepPunctuations,
TextCase = tparams.TextCase
}, view);

if (wordTokCols != null)
wordTokCols = dstCols;
else
textCols = dstCols;
}

if (tparams.NeedsRemoveStopwordsTransform)
{
Contracts.Assert(wordTokCols != null, "StopWords transform requires that word tokenization has been applied to the input text.");
Expand Down Expand Up @@ -360,7 +356,7 @@ public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataV
if (tparams.CharExtractorFactory != null)
{
{
var srcCols = wordTokCols ?? textCols;
var srcCols = tparams.NeedsRemoveStopwordsTransform ? wordTokCols : textCols;
charTokCols = new string[srcCols.Length];
var xfCols = new CharTokenizeTransform.Column[srcCols.Length];
for (int i = 0; i < srcCols.Length; i++)
Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.ML.Predictor.Tests/TestPipelineSweeper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ public void PipelineSweeperRoles()
var trainAuc = bestPipeline.PerformanceSummary.TrainingMetricValue;
var testAuc = bestPipeline.PerformanceSummary.MetricValue;
Assert.True((0.94 < trainAuc) && (trainAuc < 0.95));
Assert.True((0.83 < testAuc) && (testAuc < 0.84));
Assert.True((0.815 < testAuc) && (testAuc < 0.825));

var results = runner.GetOutput<IDataView>("ResultsOut");
Assert.NotNull(results);
Expand Down
Loading