Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

artidoro · 2019-02-01T08:10:30Z

This PR is part of the work outlined in #1798, and focuses on the OneHotHash, Hash, CopyCol, KeyToVector transformers/estimators:

Internalize constructors of estimators and transformers
Keep ColumnInfo and move it to to the estimators The ColumnInfo structure should live in the estimators, rather than transformers #1760
Rename Arguments -> Options
Internalize Options only when they are not used by public constructor. For all other cases, retain Options as public Arguments class should be made internal when possible #1758

artidoro · 2019-02-03T01:49:44Z

I have updated this PR with the feedback I received on the other PRs and have fixed it, so now it should be ready for review. #Resolved

codecov · 2019-02-03T02:25:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c544d51). Click here to learn what that means.
The diff coverage is 87.85%.

@@            Coverage Diff            @@
##             master    #2364   +/-   ##
=========================================
  Coverage          ?   71.25%           
=========================================
  Files             ?      785           
  Lines             ?   140785           
  Branches          ?    16088           
=========================================
  Hits              ?   100320           
  Misses            ?    36011           
  Partials          ?     4454

Flag	Coverage Δ
#Debug	`71.25% <87.85%> (?)`
#production	`67.61% <83.67%> (?)`
#test	`85.3% <97.61%> (?)`

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

Ivanidzo4ka · 2019-02-04T22:00:40Z

    /// </summary>

Change it to same description as you have for class #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/Hashing.cs:1232 in 3a752e3. [](commit_id = 3a752e3, deletion_comment = False)

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

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

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

Ivanidzo4ka · 2019-02-04T22:06:44Z

    public sealed class ColumnInfo

I'm surprised you haven't shuffle this class around, and didn't take care of public constructors. #Resolved

Refers to: src/Microsoft.ML.Transforms/KeyToVectorMapping.cs:44 in 3a752e3. [](commit_id = 3a752e3, deletion_comment = False)

artidoro · 2019-02-04T22:48:12Z

    public sealed class ColumnInfo

It's coming in #2367 :) I only did KeyToVector in this PR, not KeyToBinaryVector.

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

Refers to: src/Microsoft.ML.Transforms/KeyToVectorMapping.cs:44 in 5d8d49e. [](commit_id = 5d8d49e, deletion_comment = False)

artidoro · 2019-02-04T22:49:54Z

    /// </summary>

Will do!

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

Refers to: src/Microsoft.ML.Data/Transforms/Hashing.cs:1232 in 5d8d49e. [](commit_id = 5d8d49e, deletion_comment = False)

sfilipi · 2019-02-05T06:57:46Z

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

+            public readonly int HashBits;
+            /// <summary>
+            /// Hashing seed.
+            /// </summary>


nit: we can keep those in one line. ///
Hashing seed.
#Resolved

sfilipi · 2019-02-05T07:00:28Z

    /// Initializes a new instance of <see cref="HashingEstimator"/>.

i actually think this is fine for ctor comment; it seems like most .net ctor comments. For more info, you have already linked to the HashingEstimator, where there is more information available. #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/Hashing.cs:1231 in 3a752e3. [](commit_id = 3a752e3, deletion_comment = False)

sfilipi · 2019-02-05T07:02:08Z

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

+            /// </summary>
+            public readonly string Name;
+            /// <summary>
+            /// Name of column to transform. If set to <see langword="null"/>, the value of the <cref see="Name"/> will be used as source.


If set to , the value of the will be used as source. [](start = 45, length = 93)

this does not apply here. #Resolved

Very good point! Thank you :)

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

sfilipi · 2019-02-05T07:04:05Z

src/Microsoft.ML.Transforms/OneHotHashEncoding.cs

        public IRowToRowMapper GetRowToRowMapper(Schema inputSchema) => _transformer.GetRowToRowMapper(inputSchema);
    }

    /// <summary>
-    /// Estimator which takes set of columns and produce for each column indicator array. Use hashing to determine indicator position.
+    /// Estimator that produces a column of indicator vectors. The mapping between a value and a corresponding index is done through hashing.


Estimator [](start = 8, length = 9)

#Resolved

sfilipi

Ivanidzo4ka · 2019-02-05T19:16:06Z

    /// </summary>

What a lie....

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

abgoswam

Ivanidzo4ka

abgoswam · 2019-02-05T19:37:10Z

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

nit: summary comment #Resolved

Refers to: src/Microsoft.ML.Transforms/KeyToVectorMapping.cs:477 in 3a752e3. [](commit_id = 3a752e3, deletion_comment = False)

artidoro · 2019-02-05T21:34:52Z

    public override SchemaShape GetOutputSchema(SchemaShape inputSchema)

Will take care in my other PR. Here I don't go over KeyToVectorMapping closely. But thanks for looking at this.

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

Refers to: src/Microsoft.ML.Transforms/KeyToVectorMapping.cs:477 in 3a752e3. [](commit_id = 3a752e3, deletion_comment = False)

artidoro requested review from sfilipi, Ivanidzo4ka, abgoswam, zeahmed, singlis and najeeb-kazmi February 1, 2019 08:10

artidoro mentioned this pull request Feb 1, 2019

Creation of components through MLContext: advanced options and other feedback #1798

Closed

artidoro self-assigned this Feb 1, 2019

artidoro added the API Issues pertaining the friendly API label Feb 1, 2019

artidoro changed the title ~~WIP: Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector)~~ Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) Feb 2, 2019

artidoro force-pushed the hashcopyvec branch from fa7776a to 236de94 Compare February 2, 2019 02:12

Ivanidzo4ka reviewed Feb 4, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ColumnCopying.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 4, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/KeyToVector.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 4, 2019

View reviewed changes

test/Microsoft.ML.Tests/Transformers/HashTests.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 4, 2019

View reviewed changes

test/Microsoft.ML.Tests/Transformers/KeyToVectorEstimatorTests.cs Outdated Show resolved Hide resolved

artidoro added 4 commits February 4, 2019 15:49

onehothash, hash, copy col, key to vector

c428ae4

further cleaning and fix build with bestfriends assemblies

42fc780

entrypoint catalog

80c51ad

review comments

3a752e3

artidoro force-pushed the hashcopyvec branch from 5d8d49e to 3a752e3 Compare February 5, 2019 00:05

sfilipi reviewed Feb 5, 2019

View reviewed changes

sfilipi approved these changes Feb 5, 2019

View reviewed changes

abgoswam approved these changes Feb 5, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 5, 2019

View reviewed changes

review comments

ca2c603

artidoro merged commit 617f7f6 into dotnet:master Feb 5, 2019

artidoro deleted the hashcopyvec branch March 13, 2019 17:55

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

Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

artidoro commented Feb 1, 2019 •

edited

Loading

artidoro commented Feb 3, 2019 •

edited

Loading

codecov bot commented Feb 3, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 4, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 4, 2019 •

edited by artidoro

Loading

artidoro commented Feb 4, 2019

artidoro commented Feb 4, 2019

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

sfilipi commented Feb 5, 2019 •

edited by artidoro

Loading

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

artidoro Feb 5, 2019

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

sfilipi left a comment

Ivanidzo4ka commented Feb 5, 2019

abgoswam left a comment

Ivanidzo4ka left a comment

abgoswam commented Feb 5, 2019 •

edited by artidoro

Loading

artidoro commented Feb 5, 2019

Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

Creation of components through MLContext and cleanup (OneHotHash, Hash, CopyCol, KeyToVector) #2364

Conversation

artidoro commented Feb 1, 2019 • edited Loading

artidoro commented Feb 3, 2019 • edited Loading

codecov bot commented Feb 3, 2019 • edited Loading

Codecov Report

Ivanidzo4ka commented Feb 4, 2019 • edited by artidoro Loading

Ivanidzo4ka commented Feb 4, 2019 • edited by artidoro Loading

artidoro commented Feb 4, 2019

artidoro commented Feb 4, 2019

sfilipi Feb 5, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

sfilipi commented Feb 5, 2019 • edited by artidoro Loading

sfilipi Feb 5, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

artidoro Feb 5, 2019

Choose a reason for hiding this comment

sfilipi Feb 5, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 5, 2019

abgoswam left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

abgoswam commented Feb 5, 2019 • edited by artidoro Loading

artidoro commented Feb 5, 2019

artidoro commented Feb 1, 2019 •

edited

Loading

artidoro commented Feb 3, 2019 •

edited

Loading

codecov bot commented Feb 3, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 4, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 4, 2019 •

edited by artidoro

Loading

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

sfilipi commented Feb 5, 2019 •

edited by artidoro

Loading

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

sfilipi Feb 5, 2019 •

edited by artidoro

Loading

abgoswam commented Feb 5, 2019 •

edited by artidoro

Loading