Metadata fixes for the ValueMappingEstimator #2098

singlis · 2019-01-09T22:50:03Z

The ValueMappingEstimator had a couple of issues when using the Values as KeyTypes:

The output schema for the Estimator did not contain the KeyType
information in the metadata.
The reverse lookup of the metadata had the incorrect value.

This now sets the correct metadata on the output schema and uses the
value data for the reverse lookup. A test was added to confirm the
changes using the KeyToValueMapping appended to a ValueMappingEstimator
for the reverse lookup.

Fixes #2086
Fixes #2083

Values as KeyTypes: 1) The output schema for the Estimator did not contain the KeyType information in the metadata. 2) The reverse lookup of the metadata had the incorrect value. This now sets the correct metadata on the output schema and uses the value data for the reverse lookup. A test was added to confirm the changes using the KeyToValueMapping appended to a ValueMappingEstimator for the reverse lookup. Fixes dotnet#2086 Fixes dotnet#2083

…n a Host.Assert and would not execute on release build. This is changed to a Host.Check

Ivanidzo4ka · 2019-01-11T01:06:45Z

src/Microsoft.ML.Data/Transforms/ValueMappingTransformer.cs

@@ -191,18 +192,15 @@ private static ValueGetter<VBuffer<ReadOnlyMemory<char>>> GetKeyValueGetter<TKey
                // set of values. This is used for generating the metadata of
                // the column.
                HashSet<TValue> valueSet = new HashSet<TValue>();
-                HashSet<TKey> keySet = new HashSet<TKey>();
                for (int i = 0; i < values.Count(); ++i)
                {
                    var v = values.ElementAt(i);


Can you replace it with foreach loop?
I found terrifying idea of fetching i-th element every time out of IEnumerable #Closed

absolutely.

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

Ivanidzo4ka · 2019-01-11T01:08:26Z

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

+
+            var result = t.Transform(dataView);
+            var cursor = result.GetRowCursor((col) => true);
+            var getterD = cursor.GetGetter<ReadOnlyMemory<char>>(6);


6 [](start = 65, length = 1)

magic numbers, can you do result.Schema["DOutput"].Index? #Closed

Ivanidzo4ka · 2019-01-11T01:09:26Z

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

+            var data = new[] { new TestClass() { A = "bar", B = "test", C = "notfound" } };
+            var dataView = ComponentCreation.CreateDataView(Env, data);
+
+            IEnumerable<ReadOnlyMemory<char>> keys = new List<ReadOnlyMemory<char>>() { "foo".AsMemory(), "bar".AsMemory(), "test".AsMemory(), "wahoo".AsMemory() };


IEnumerable<ReadOnlyMemory> [](start = 12, length = 33)

what's wrong with var? #Closed

Ivanidzo4ka · 2019-01-11T01:12:01Z

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

+
+            // The expected values will contain the generated key type values starting from 1.
+            ReadOnlyMemory<char> dValue = default;
+            getterD(ref dValue);


dValue [](start = 24, length = 6)

can we validate it's a "bar"? #Closed

Ivanidzo4ka

wschin · 2019-01-11T23:21:50Z

src/Microsoft.ML.Data/Transforms/ValueMappingTransformer.cs

@@ -100,13 +100,14 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
            var isKey = Transformer.ValueColumnType.IsKey;
            var columnType = (isKey) ? PrimitiveType.FromKind(DataKind.U4) :
                                    Transformer.ValueColumnType;
+            var metadata = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);


Suggested change

var metadata = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);

var metadataShape = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);

Maybe? #Resolved

Sure, I will update.

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

wschin · 2019-01-11T23:22:52Z

src/Microsoft.ML.Data/Transforms/ValueMappingTransformer.cs

@@ -100,13 +100,14 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
            var isKey = Transformer.ValueColumnType.IsKey;
            var columnType = (isKey) ? PrimitiveType.FromKind(DataKind.U4) :
                                    Transformer.ValueColumnType;
+            var metadata = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);
            foreach (var (Input, Output) in _columns)
            {
                if (!inputSchema.TryFindColumn(Input, out var originalColumn))
                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", Input);

                // Get the type from TOutputType


What is TOutputType here? Is it not Output? #Resolved

That is old, I used to have the generic arguments be TInputType, TOutputType, but then changed to TKeyType and TValueType. I will update the comment.

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

wschin · 2019-01-11T23:23:48Z

src/Microsoft.ML.Data/Transforms/ValueMappingTransformer.cs

            foreach (var (Input, Output) in _columns)
            {
                if (!inputSchema.TryFindColumn(Input, out var originalColumn))
                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", Input);

                // Get the type from TOutputType
-                var col = new SchemaShape.Column(Output, vectorKind, columnType, isKey, originalColumn.Metadata);
+                var col = new SchemaShape.Column(Output, vectorKind, columnType, isKey, metadata);


Suggested change

var col = new SchemaShape.Column(Output, vectorKind, columnType, isKey, metadata);

var col = new SchemaShape.Column(OutputColumnName, vectorKind, columnType, isKey, metadata);

``` #WontFix

Although Name is more specific to what the object actually is, I think it goes against thinking of these as columns since you are talking about a input/output (or source/name). I would prefer to keep it as Output/Input. BTW - this will also go through another revision with #2064

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

wschin · 2019-01-11T23:33:06Z

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

+            var values = new List<ReadOnlyMemory<char>>() { "foo1".AsMemory(), "foo2".AsMemory(), "foo1".AsMemory(), "foo3".AsMemory() };
+
+            var estimator = new ValueMappingEstimator<ReadOnlyMemory<char>, ReadOnlyMemory<char>>(Env, keys, values, true, new[] { ("A", "D"), ("B", "E"), ("C", "F") })
+                            .Append(new KeyToValueMappingEstimator(Env, ("D","DOutput")));


Suggested change

.Append(new KeyToValueMappingEstimator(Env, ("D","DOutput")));

.Append(new KeyToValueMappingEstimator(Env, ("D", "DOutput")));

Please format all files touched. #Resolved

Thanks, they are both formatted now.

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

wschin · 2019-01-11T23:39:23Z

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

+            // Generating the list of strings for the key type values, note that foo1 is duplicated as intended to test that the same index value is returned
+            var values = new List<ReadOnlyMemory<char>>() { "foo1".AsMemory(), "foo2".AsMemory(), "foo1".AsMemory(), "foo3".AsMemory() };
+
+            var estimator = new ValueMappingEstimator<ReadOnlyMemory<char>, ReadOnlyMemory<char>>(Env, keys, values, true, new[] { ("A", "D"), ("B", "E"), ("C", "F") })


Why do you need three columns? Only "D" is used below. #Resolved

True - that is a result of copying code from another test. I have removed the other two columns.

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

singlis requested review from Ivanidzo4ka, wschin and sfilipi January 9, 2019 22:50

-Fixing runs for release build. Code that needed to run was wrapped i…

904975a

…n a Host.Assert and would not execute on release build. This is changed to a Host.Check

Ivanidzo4ka reviewed Jan 11, 2019

View reviewed changes

singlis added 2 commits January 11, 2019 11:02

Merge remote-tracking branch 'origin/master' into singlis/valuemap-fix

3b3f6da

- Updating changes based upon feedback.

c4729ff

Ivanidzo4ka approved these changes Jan 11, 2019

View reviewed changes

wschin reviewed Jan 11, 2019

View reviewed changes

singlis self-assigned this Jan 14, 2019

singlis added 2 commits January 14, 2019 09:04

Merge remote-tracking branch 'origin/master' into singlis/valuemap-fix

5759218

- Updating from feedback

fa27d84

wschin approved these changes Jan 15, 2019

View reviewed changes

singlis merged commit ef638f4 into dotnet:master Jan 15, 2019

singlis deleted the singlis/valuemap-fix branch March 7, 2019 00:54

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metadata fixes for the ValueMappingEstimator #2098

Metadata fixes for the ValueMappingEstimator #2098

singlis commented Jan 9, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

singlis Jan 11, 2019

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka left a comment

wschin Jan 11, 2019 •

edited by singlis

Loading

singlis Jan 14, 2019

wschin Jan 11, 2019 •

edited by singlis

Loading

singlis Jan 14, 2019

wschin Jan 11, 2019 •

edited by singlis

Loading

singlis Jan 14, 2019

wschin Jan 11, 2019 •

edited by singlis

Loading

singlis Jan 14, 2019

wschin Jan 11, 2019 •

edited by singlis

Loading

singlis Jan 14, 2019

	var metadata = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);
	var metadataShape = SchemaShape.Create(Transformer.ValueColumnMetadata.Schema);

	var col = new SchemaShape.Column(Output, vectorKind, columnType, isKey, metadata);
	var col = new SchemaShape.Column(OutputColumnName, vectorKind, columnType, isKey, metadata);
	``` #WontFix

	.Append(new KeyToValueMappingEstimator(Env, ("D","DOutput")));
	.Append(new KeyToValueMappingEstimator(Env, ("D", "DOutput")));

Metadata fixes for the ValueMappingEstimator #2098

Metadata fixes for the ValueMappingEstimator #2098

Conversation

singlis commented Jan 9, 2019 • edited Loading

Ivanidzo4ka Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

singlis Jan 11, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

wschin Jan 11, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Jan 14, 2019

Choose a reason for hiding this comment

wschin Jan 11, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Jan 14, 2019

Choose a reason for hiding this comment

wschin Jan 11, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Jan 14, 2019

Choose a reason for hiding this comment

wschin Jan 11, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Jan 14, 2019

Choose a reason for hiding this comment

wschin Jan 11, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Jan 14, 2019

Choose a reason for hiding this comment

singlis commented Jan 9, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

Ivanidzo4ka Jan 11, 2019 •

edited

Loading

wschin Jan 11, 2019 •

edited by singlis

Loading

wschin Jan 11, 2019 •

edited by singlis

Loading

wschin Jan 11, 2019 •

edited by singlis

Loading

wschin Jan 11, 2019 •

edited by singlis

Loading

wschin Jan 11, 2019 •

edited by singlis

Loading