Convert categorical hash to estimator #1033

Ivanidzo4ka · 2018-09-26T00:18:34Z

Convert Cathash to estimator

Zruty0 · 2018-09-26T00:52:57Z

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

@@ -116,7 +116,7 @@ public bool TryUnparse(StringBuilder sb)
            }
        }

-        public sealed class ColumnInfo


sealed [](start = 15, length = 6)

If it's not sealed, it should be abstract #Resolved

Zruty0 · 2018-09-26T00:53:52Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

-                OutputKind = outputKind
-            };
-            return Create(env, args, input);
+            return new CategoricalHashEstimator(env, name, source, outputKind).Fit(input).Transform(input) as IDataView;
        }

        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


public [](start = 8, length = 6)

can we make this private? #Closed

We call it in entrypoint, so internal

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

Zruty0 · 2018-09-26T00:55:13Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

+        public CategoricalHashTransform(HashEstimator hash, IEstimator<ITransformer> keyToVector, IDataView input)
+        {
+            var chain = hash.Append(keyToVector);
+            _transformer = chain.Fit(input);


this code belongs to the caller I think. I don't feel comfortable having a public ctor for transformer that takes estimators as args. Also, there should not be a public ctor for the transformer, if it's traiunable. #Closed

I'm not getting your message, but I can make constructor internal.

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

Ok, good enough.

In reply to: 221013014 [](ancestors = 221013014,220399096)

Zruty0 · 2018-09-26T00:56:02Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

+        public bool IsRowToRowMapper => _transformer.IsRowToRowMapper;
+
+        public IRowToRowMapper GetRowToRowMapper(ISchema inputSchema)
+        {


{ [](start = 8, length = 1)

=> as well? It should check #Closed

what?

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

I mean the inner method will check for the schema anyway
so you can use => here

In reply to: 221013063 [](ancestors = 221013063,220399209)

Zruty0 · 2018-09-26T00:56:15Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

+        }
+    }
+
+    public sealed class CategoricalHashEstimator : IEstimator<CategoricalHashTransform>


public [](start = 4, length = 6)

comment #Closed

Zruty0 · 2018-09-26T00:56:20Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

+
+    public sealed class CategoricalHashEstimator : IEstimator<CategoricalHashTransform>
+    {
+        public static class Defaults


public [](start = 8, length = 6)

internal? #Closed

Zruty0 · 2018-09-26T00:57:18Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

-        private static IDataTransform CreateTransformCore(CategoricalTransform.OutputKind argsOutputKind, OneToOneColumn[] columns,
-            List<CategoricalTransform.OutputKind?> columnOutputKinds, IDataTransform input, IHost h, Arguments catHashArgs = null)
+        private readonly IHost _host;
+        private readonly IEstimator<ITransformer> _keyToSomething;


_keyToSomething [](start = 50, length = 15)

let's call it 'toVector' :) #Closed

Zruty0 · 2018-09-26T00:58:44Z

                        binaryEncoding = true;

the logic is 'if ANY output is binary, then ALL is binary', right? Doesn't sound right to me #Closed

Refers to: src/Microsoft.ML.Transforms/CategoricalHashTransform.cs:259 in cc70940. [](commit_id = cc70940, deletion_comment = False)

Zruty0 · 2018-09-26T00:59:43Z

                        binaryEncoding = true;

I know that it's how it used to work, but I think we should do better than this. Either have 2 estimators for different sets of columns, or throw.

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

Refers to: src/Microsoft.ML.Transforms/CategoricalHashTransform.cs:259 in cc70940. [](commit_id = cc70940, deletion_comment = False)

Zruty0 · 2018-09-26T01:02:02Z

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

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


CategoricalHashStatic [](start = 20, length = 21)

move to static pipeline tests assembly #Pending

Can you create separate issue, since we have bunch (probably most of them) in Tests now?

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

Zruty0 · 2018-09-26T01:02:19Z

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

+            var data = new[] {
+                new TestMeta() { A=new string[2] { "A", "B"}, B="C", C= new float[2] { 1.0f,2.0f}, D = 1.0f , E= new string[2]{ "A","D"}, F="D"},
+                new TestMeta() { A=new string[2] { "A", "B"}, B="C",C=new float[2] { 3.0f,4.0f}, D = -1.0f ,E= new string[2]{"E", "A"}, F="E"},
+                new TestMeta() { A=new string[2] { "A", "B"}, B="C",C=new float[2] { 5.0f,6.0f}, D = 1.0f ,E= new string[2]{ "D", "E"}, F="D"} };


F="D" [](start = 136, length = 5)

spacing #Closed

Zruty0

sfilipi · 2018-09-28T20:06:42Z

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

+            using (var ch = Env.Start("save"))
+            {
+                var saver = new TextSaver(Env, new TextSaver.Arguments { Silent = true });
+                IDataView savedData = TakeFilter.Create(Env, est.Fit(data).Transform(data).AsDynamic, 4);


IDataView [](start = 16, length = 9)

var #Resolved

sfilipi · 2018-09-28T20:14:42Z

src/Microsoft.ML.Transforms/CategoricalTransform.cs

@@ -171,16 +171,14 @@ public CategoricalTransform(TermEstimator term, IEstimator<ITransformer> keyToVe

        public bool IsRowToRowMapper => _transformer.IsRowToRowMapper;

-        public IRowToRowMapper GetRowToRowMapper(ISchema inputSchema)
-        {
-            Contracts.CheckValue(inputSchema, nameof(inputSchema));


Contrac [](start = 11, length = 8)

did you intend to omit the check? #Resolved

As @Zruty0 pointed out we run check inside _transformer.GetRowToRowMapper

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

sfilipi · 2018-09-28T20:16:10Z

src/Microsoft.ML.Transforms/CategoricalTransform.cs

@@ -455,13 +453,13 @@ private sealed class Rec : EstimatorReconciler
        }

        /// <summary>
-        /// Converts the categorical value into an indicator array by building a dictionary of categories based on the data and using the id in the dictionary as the index in the array
+        /// Converts the categorical value into an indicator array by building a dictionary of categories based on the data and using the id in the dictionary as the index in the array.
        /// </summary>
        /// <param name="input">Incoming data.</param>
        /// <param name="outputKind">Specify output type of indicator array: array or binary encoded data.</param>
        /// <param name="order">How Id for each value would be assigined: by occurrence or by value.</param>


How Id [](start = 31, length = 7)

think it would read better: "How the Id" #Resolved

sfilipi · 2018-09-28T20:16:23Z

src/Microsoft.ML.Transforms/CategoricalTransform.cs

@@ -455,13 +453,13 @@ private sealed class Rec : EstimatorReconciler
        }

        /// <summary>
-        /// Converts the categorical value into an indicator array by building a dictionary of categories based on the data and using the id in the dictionary as the index in the array
+        /// Converts the categorical value into an indicator array by building a dictionary of categories based on the data and using the id in the dictionary as the index in the array.
        /// </summary>
        /// <param name="input">Incoming data.</param>
        /// <param name="outputKind">Specify output type of indicator array: array or binary encoded data.</param>


Specify output type [](start = 37, length = 19)

think it would read better: "Specify the output" #Resolved

sfilipi

Zruty0 · 2018-10-03T00:25:37Z

src/Microsoft.ML.Transforms/CategoricalHashTransform.cs

-                    col.Source = column.Name;
-                    col.Bag = bag;
-                    cols.Add(col);
+                    if (kind == CategoricalTransform.OutputKind.Bin)


if [](start = 20, length = 2)

fold the if into the switch above?

cat hash

cc70940

Ivanidzo4ka requested review from Zruty0 and TomFinley September 26, 2018 00:18

Zruty0 reviewed Sep 26, 2018

View reviewed changes

Zruty0 approved these changes Sep 26, 2018

View reviewed changes

Ivan Matantsev added 5 commits September 25, 2018 22:58

Merge branch 'master' into Ivanidze/CatHash

0dfd2eb

fix build

ab8ba2c

some comments

f23b54c

add comments for estimator

30080e2

more comments

3f8de2d

Ivanidzo4ka requested a review from sfilipi September 28, 2018 18:10

sfilipi reviewed Sep 28, 2018

View reviewed changes

sfilipi approved these changes Sep 28, 2018

View reviewed changes

Ivan Matantsev added 3 commits September 28, 2018 13:25

address Senja comments

4c78550

the

8aec157

break apart bin logic

b581c38

Zruty0 reviewed Oct 3, 2018

View reviewed changes

Ivan Matantsev added 2 commits October 2, 2018 18:02

move to switch

71a1115

build!

16db6a1

Ivanidzo4ka merged commit c6c0d22 into dotnet:master Oct 3, 2018

Zruty0 mentioned this pull request Oct 4, 2018

New API for ML.NET #754

Closed

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 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert categorical hash to estimator #1033

Convert categorical hash to estimator #1033

Ivanidzo4ka commented Sep 26, 2018

Zruty0 Sep 26, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Ivanidzo4ka Sep 27, 2018

Zruty0 Sep 26, 2018 •

edited

Loading

Ivanidzo4ka Sep 27, 2018

Zruty0 Oct 2, 2018

Zruty0 Sep 26, 2018 •

edited

Loading

Ivanidzo4ka Sep 27, 2018

Zruty0 Sep 28, 2018

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 commented Sep 26, 2018 •

edited

Loading

Zruty0 commented Sep 26, 2018

Zruty0 Sep 26, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 27, 2018

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 left a comment

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Sep 28, 2018

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi left a comment

Zruty0 Oct 3, 2018

Convert categorical hash to estimator #1033

Convert categorical hash to estimator #1033

Conversation

Ivanidzo4ka commented Sep 26, 2018

Zruty0 Sep 26, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Sep 26, 2018 • edited Loading

Zruty0 commented Sep 26, 2018

Zruty0 Sep 26, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

sfilipi Sep 28, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Sep 28, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 28, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi Sep 28, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Sep 26, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

Zruty0 commented Sep 26, 2018 •

edited

Loading

Zruty0 Sep 26, 2018 •

edited by Ivanidzo4ka

Loading

Zruty0 Sep 26, 2018 •

edited

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading

sfilipi Sep 28, 2018 •

edited by Ivanidzo4ka

Loading