KeyToVector estimators #858

Ivanidzo4ka · 2018-09-07T22:15:53Z

Move KeyToVector to estimators land

Ivanidzo4ka · 2018-09-07T22:17:09Z

src/Microsoft.ML.Transforms/CategoricalTransform.cs

@@ -232,7 +232,7 @@ public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataV
                        Column = cols.ToArray()
                    };

-                    transform = new KeyToVectorTransform(h, keyToVecArgs, input);
+                    transform =KeyToVectorTransform.Create(h, keyToVecArgs, input);


KeyToVectorTransform [](start = 31, length = 20)

move to non arguments creator #Closed

Ivanidzo4ka · 2018-09-07T22:18:01Z

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

+                Bag = bag;
+            }
+        }
+        internal sealed class ColInfo


internal [](start = 8, length = 8)

new line #Resolved

Ivanidzo4ka · 2018-09-07T22:19:17Z

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

            {
-                if (bag || info.TypeSrc.ValueCount == 1)
+                InputSchema.TryGetColumnIndex(_infos[i].Source, out int srcCol);
+                //IVAN: Simplify


IVAN [](start = 18, length = 4)

get rid of //IVAN #Resolved

Don't get rid of Ivan. He's such a useful, competent person to have around.

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

Ivanidzo4ka · 2018-09-08T00:49:08Z

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

-                    // Output is the concatenation of the multiple output indicator vectors.
-                    concat = true;
-                    type = new VectorType(NumberType.Float, info.TypeSrc.ValueCount, size);
+                    var type = new VectorType(NumberType.Float, _parent._valueCounts[i], _parent._sizes[i]);
                    if (typeNames != null && type.VectorSize > 0)


VectorSize > 0 [](start = 50, length = 14)

IsKnownSizeVector #Resolved

sfilipi · 2018-09-08T05:27:33Z

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


-        // These arrays are parallel to Infos.
-        // * _bag indicates whether vector inputs should have their output indicator vectors added
-        //   (instead of concatenated). This is faithful to what the user specified in the Arguments


why remove the comment? #Resolved

I left comment for bags, everything is no longer exist in code

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

sfilipi · 2018-09-08T05:43:53Z

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

        }

-        public override bool CanSavePfa => true;
-        public override bool CanSaveOnnx => true;
+        // Factory method for SignatureLoadModel.


// Factor [](start = 8, length = 9)

Convert it to XML doc, since it is a public method (and if there is anything more to add)

sfilipi · 2018-09-08T05:45:18Z

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

+        public static IDataTransform Create(IHostEnvironment env, IDataView input, params ColumnInfo[] columns) =>
+             new KeyToVectorTransform(env, input, columns).MakeDataTransform(input);
+
+        // Factory method for SignatureDataTransform.


// Fa [](start = 8, length = 5)

same here. #ByDesign

I'll rather make component catalog to support internal Create methods

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

TomFinley · 2018-09-08T20:56:32Z

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

+            return new VersionInfo(
+                modelSignature: "KEY2VECT",
+                //verWrittenCur: 0x00010001, // Initial
+                verWrittenCur: 0x00010002, // Convert to Estimators


verWrittenCur: 0x00010002, // Convert to Estimators [](start = 16, length = 51)

As far as I know it has not been necessary to break backwards compatibility so far, why is it necessary here? #Closed

No more backward incompatible.

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

TomFinley

Thanks for looking at this @Ivanidzo4ka . Key to vector, as the final step for much of our categorical and text processing pipelines, is one of the most important transforms in our repertoire. By breaking backwards compatibility, we will break many prior models, which does not seem like a good move. Let us try to imagine a way, as we have with all other transforms as far as I am aware, to have the old models successfully load as transformers now.

Zruty0 · 2018-09-10T18:26:06Z

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

-                ComputeType(this, Source.Schema, i, Infos[i], _bag[i], Metadata,
-                    out _types[i], out _concat[i]);
+                if (!schema.TryGetColumnIndex(ColumnPairs[i].input, out int colSrc))
+                    throw Host.ExceptUserArg(nameof(ColumnPairs), "Source column '{0}' not found", ColumnPairs[i].input);


ExceptUserArg [](start = 31, length = 13)

use ExceptSchemaMismatch #Closed

Zruty0 · 2018-09-10T18:27:09Z

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

+        }
+    }
+
+    public sealed class KeyToVectorEstimator : IEstimator<KeyToVectorTransform>


IEstimator [](start = 47, length = 10)

we agreed to make this estimator a trivial estimator, and not enforce the match between train-time schema and transform-time schema #Closed

Zruty0 · 2018-09-10T18:28:03Z

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

+
+            var pipe = new KeyToVectorEstimator(Env, new KeyToVectorTransform.ColumnInfo("TermA", "CatA", false),
+                new KeyToVectorTransform.ColumnInfo("TermB", "CatB", true));
+            TestEstimatorCore(pipe, dataView);


TestEstimatorCore [](start = 12, length = 17)

Make sure to call Done() when you are done #Closed

Zruty0 · 2018-09-10T18:28:32Z

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

+            var result = pipe.Fit(dataView).Transform(dataView);
+            ValidateMetadata(result);
+        }
+        void ValidateMetadata(IDataView result)


void [](start = 8, length = 4)

spacing, and explicit private #Closed

Zruty0 · 2018-09-10T18:28:54Z

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

+        }
+
+        [Fact]
+        void TestCommandLine()


void [](start = 8, length = 4)

explicit public as well #Closed

Zruty0

🕐

Zruty0 · 2018-09-10T20:26:48Z

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

-            SaveBase(ctx);
+            // for each added column
+            //  int: keyCount
+            //  int: valueCount


remove now? #Closed

Ivanidzo4ka · 2018-09-10T20:30:14Z

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

@@ -241,6 +241,13 @@ private static (string input, string output)[] GetColumnPairs(ColumnInfo[] colum
            return columns.Select(x => (x.Input, x.Output)).ToArray();
        }

+        internal static string TestIsKnownDataKind(ColumnType type)


static [](start = 17, length = 6)

don't have to be static #Resolved

Zruty0 · 2018-09-10T22:05:53Z

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

+
+            var result = pipe.Fit(dataView).Transform(dataView);
+            ValidateMetadata(result);
+        }


} [](start = 7, length = 2)

Done() #Resolved

what for?

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

It's the contract. Just in case you want to eventually add a baseline comparison here, it's better to end every test in TestDataPipeBase with Done()

In reply to: 216488662 [](ancestors = 216488662,216488081)

Zruty0

Ivanidzo4ka · 2018-09-11T16:24:34Z

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

        /// </summary>
-        private ValueGetter<VBuffer<Float>> MakeGetterInd(IRow input, int iinfo)
+        public static Vector<TValue> ToVector<TKey, TValue>(this Key<TKey, TValue> input, bool bag = DefaultBag)


TValue [](start = 29, length = 6)

Should be float #Closed

Zruty0 · 2018-09-11T17:17:50Z

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

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


Pigsty [](start = 31, length = 6)

Rename to 'static' #Resolved

Zruty0 · 2018-09-11T17:18:11Z

src/Microsoft.ML.Transforms/KeyToBinaryVectorTransform.cs

+    /// <summary>
+    /// Extension methods for the static-pipeline over <see cref="PipelineColumn"/> objects.
+    /// </summary>
+    public static class KeyToVectorExtensions


Vector [](start = 29, length = 6)

BinaryVector ? #Resolved

Zruty0 · 2018-09-11T17:18:49Z

src/Microsoft.ML.Transforms/KeyToBinaryVectorTransform.cs

        }

-        private void EncodeValueToBinary(BufferBuilder<float> bldr, uint value, int bitsToConsider, int startIndex)
+        /// <summary>
+        /// Takes a column of key type of known cardinality and produces a binary encoded indicator vector of zeros and ones.


binary encoded indicator vector of zeros and ones [](start = 75, length = 49)

a vector of bits representing the key in binary form. #Resolved

revoking review

TomFinley · 2018-09-11T19:54:29Z

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

        /// </summary>
-        private ValueGetter<VBuffer<Float>> MakeGetterInd(IRow input, int iinfo)
+        public static Vector<float> ToVector<TKey, TValue>(this Key<TKey, TValue> input, bool bag = DefaultBag)


bool bag = DefaultBag [](start = 89, length = 21)

Is bagging a sensible parameter on a scalar value? Nothing is being combined with anything else -- there's only one thing. #Closed

TomFinley · 2018-09-11T19:55:04Z

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

+        /// <summary>
+        /// Takes a column of key type of known cardinality and produces an indicator vector of floats.
+        /// </summary>
+        public static Vector<float> ToVector<TKey, TValue>(this Vector<Key<TKey, TValue>> input, bool bag = DefaultBag)


public static Vector ToVector<TKey, TValue>(this Vector<Key<TKey, TValue>> input, bool bag = DefaultBag) [](start = 8, length = 111)

One of the most important usages of KeyToVector comes in the VarVector input case (e.g., text processing). It is crucially important that we support that scenario. In that case if you wanted to support bagging vs. not, you will need two overloads. I suggest to name one of them ToVector (no bagging, returns VarVector<float>), and the other to name it ToBaggedVector (bagging, returns Vector<float>).

Because of this distinction is necessary for the VarVector input case (which is again really important), I suggest we retain the distiction for consitency sake for the fixed size vector case, for reasons of user consistency. #Resolved

TomFinley · 2018-09-11T19:56:33Z

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

        }
+
+        /// <summary>
+        /// Takes a column of key type of known cardinality and produces an indicator vector of floats.


Takes a column of key type of known cardinality and produces an indicator vector of floats. [](start = 12, length = 91)

This description is insufficient. Do you feel like you could write better documentation, more in line with the other Pigsty extension methods? It is important for our public API that we get that right.

A sufficient description would describe what is happening to the data. Also crucial is a discussion on what happens in bagged vs. not. #Pending

Is this better?

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

TomFinley · 2018-09-11T19:58:20Z

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

        /// </summary>
-        private ValueGetter<VBuffer<Float>> MakeGetterInd(IRow input, int iinfo)
+        public static Vector<float> ToVector<TKey, TValue>(this Key<TKey, TValue> input, bool bag = DefaultBag)


Key<TKey, TValue> [](start = 64, length = 17)

Hi @Ivanidzo4ka. I suggest you change the input type from Key<TKey, TValue> to just plain old Key<T>. Whether or not there is key-value metadata is immaterial to the functioning of this transform, and Key<T,V> descends from Key<T> anyway. #Closed

is this comment only for this case, or in general across all methods?

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

Everything ought to allow a value of Key<T> and Key<T,V>, by some mechanism. I had, however, forgotten about Vector<Key<T,V>> not being a Vector<Key<T>>... this will require a separate overload I think.

In reply to: 216812660 [](ancestors = 216812660,216801604)

Ok, I added Key and Key<TKey,TValue> extensions

In reply to: 216841257 [](ancestors = 216841257,216812660,216801604)

TomFinley · 2018-09-11T19:59:08Z

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

-        /// </summary>
-        private ValueGetter<VBuffer<Float>> MakeGetterBag(IRow input, int iinfo)
+    /// <summary>
+    /// Extension methods for the static-pipeline over <see cref="PipelineColumn"/> objects.


static-pipeline over objects [](start = 34, length = 57)

I I think you meant static pipelines over the key-to-vector transform, right?

I just copied this line from other code in hope what this is common pattern. Is it not the case?

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

If it is I or someone did not provide the most helpful message. But if so, I do not view it as terribly consequential -- after using it day in day out for the better part of a decade I could not honestly tell you I have ever once read the documentation of whatever class provides the LINQ extension methods, though assuredly I have used them many thousands of times. And if I can do so for this, what chance would I or anyone have of seeing this doc? Indeed you could put something incredibly NSFW here and I'm almost certain no user would ever know. And this writing is not wrong, just kinda unspecific. So I guess it doesn't matter.

In reply to: 216812383 [](ancestors = 216812383,216801837)

TomFinley · 2018-09-11T20:00:03Z

Hi @[email protected] thanks for this, just a few more necessary changes.

TomFinley

Thank you very much @Ivanidzo4ka , this appears quite nice.

TomFinley

TomFinley

Thank you @Ivanidzo4ka. This is an essential piece of the puzzle, arguably our most important mechanism for featurization.

Ivan Matantsev added 5 commits September 7, 2018 10:51

still need metadata tests

b1f3d69

Merge branch 'master' into ivanidze/keytovectorEstimators

599310b

merge with master

b8ce664

Merge branch 'master' into ivanidze/keytovectorEstimators

9485a9c

Improve tests

8eb2691

Ivanidzo4ka commented Sep 7, 2018

View reviewed changes

cleanup code a bit

b17d003

Ivanidzo4ka commented Sep 8, 2018

View reviewed changes

sfilipi reviewed Sep 8, 2018

View reviewed changes

TomFinley reviewed Sep 8, 2018

View reviewed changes

TomFinley previously requested changes Sep 8, 2018

View reviewed changes

Ivanidzo4ka added the API Issues pertaining the friendly API label Sep 9, 2018

Ivanidzo4ka added this to the 0918 milestone Sep 9, 2018

address comments

d541c64

Zruty0 assigned Ivanidzo4ka Sep 10, 2018

Zruty0 mentioned this pull request Sep 10, 2018

New API for ML.NET #754

Closed

Zruty0 reviewed Sep 10, 2018

View reviewed changes

Zruty0 suggested changes Sep 10, 2018

View reviewed changes

KeyToBinary estimator

28c8d13

Zruty0 reviewed Sep 10, 2018

View reviewed changes

address some comments

29ec055

Ivanidzo4ka commented Sep 10, 2018

View reviewed changes

move to trivial estimator

91e60ed

Zruty0 reviewed Sep 10, 2018

View reviewed changes

Zruty0 approved these changes Sep 10, 2018

View reviewed changes

Ivan Matantsev added 2 commits September 10, 2018 15:57

I ain't afraid of no pigs!

f71b159

small fixes

bbbc714

Ivanidzo4ka commented Sep 11, 2018

View reviewed changes

Zruty0 reviewed Sep 11, 2018

View reviewed changes

address comments

a21832e

TomFinley reviewed Sep 11, 2018

View reviewed changes

Ivan Matantsev added 4 commits September 11, 2018 13:51

address some Tom comments

ac641d6

some more work

27c20e0

Merge branch 'master' into ivanidze/keytovectorEstimators

c09bd8a

Key<TKey> and Key<TKey,TValue>

d4abb9b

TomFinley approved these changes Sep 11, 2018

View reviewed changes

Ivan Matantsev added 3 commits September 11, 2018 16:47

merge conflicts

42305de

now proper merge

bbc2dc2

Fix tests

191e5ce

TomFinley approved these changes Sep 12, 2018

View reviewed changes

TomFinley merged commit 356cad4 into dotnet:master Sep 12, 2018

sfilipi mentioned this pull request Oct 10, 2018

API reference - Samples for Transforms #1209

Closed

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

KeyToVector estimators #858

KeyToVector estimators #858

Conversation

Ivanidzo4ka commented Sep 7, 2018

Ivanidzo4ka Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 8, 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

sfilipi Sep 8, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley left a comment • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Zruty0 Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 10, 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

Zruty0 left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 11, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Sep 11, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Sep 11, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Sep 11, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Sep 11, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Sep 11, 2018

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley left a comment • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Sep 7, 2018 •

edited

Loading

Ivanidzo4ka Sep 7, 2018 •

edited

Loading

Ivanidzo4ka Sep 7, 2018 •

edited

Loading

Ivanidzo4ka Sep 8, 2018 •

edited

Loading

sfilipi Sep 8, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 8, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Sep 8, 2018 •

edited

Loading

TomFinley left a comment •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited

Loading

Ivanidzo4ka Sep 10, 2018 •

edited

Loading

Zruty0 Sep 10, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 11, 2018 •

edited

Loading

Zruty0 Sep 11, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 11, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 11, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Sep 11, 2018 •

edited

Loading

TomFinley Sep 11, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Sep 11, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Sep 11, 2018 •

edited

Loading

TomFinley Sep 12, 2018 •

edited

Loading

TomFinley left a comment •

edited

Loading