Normalization API helpers by TomFinley · Pull Request #446 · dotnet/machinelearning

TomFinley · 2018-06-28T17:52:19Z

In which I introduce some helpers for normalization, and generally try to clean up the code-base. Addresses #433 . Hopefully will be used in #424, though I've changed the code here to use some of it where it made sense.

The Microsoft.ML.Data transform had a "hidden" dependency on Microsoft.ML.Transform project via dependency injection, for its existing "helper" for normalization (the console-app centric version). This has been resolved and replaced with direct instantiation. It required moving the normalizer files, however.
Introduction of helpers on NormalizeTransform for API-centric operations. (Not necessarily useful directly for console-application/GUI usage.)
Some documentation changes on RoleMappedSchema and RoleMappedData, though more non-cosmetic changes I'd expected would come with Direct API: RoleMappedSchema/Data Cleanup, Improvement #445 .

TomFinley · 2018-06-28T17:55:25Z

src/Microsoft.ML.Data/Commands/TrainCommand.cs

-                    view = CompositeDataLoader.Create(env, loader,
-                        new KeyValuePair<string, SubComponent<IDataTransform, SignatureDataTransform>>(null, component));
-                }
+                // REVIEW: This verbose constructor should be replaced with zeahmed's enhancements once #405 is committed.


As mentioned, can be made more brief with #405. #Closed

Ivanidzo4ka · 2018-06-28T17:57:04Z

src/Microsoft.ML.Data/Commands/TrainCommand.cs

-                    view = CompositeDataLoader.Create(env, loader,
-                        new KeyValuePair<string, SubComponent<IDataTransform, SignatureDataTransform>>(null, component));
-                }
+                // REVIEW: This verbose constructor should be replaced with zeahmed's enhancements once #405 is committed.


zeahmed [](start = 76, length = 7)

I though we have tendency to remove aliases from codebase... #Resolved

Ivanidzo4ka · 2018-06-28T18:04:14Z

                    groupInfo = ColumnInfo.CreateFromName(data.Schema, convCol.Name, "converted group id");

do we also need this line? it just if you remove labelInfo, groupInfo looks weird if it remaind untouched. #Resolved

Refers to: src/Microsoft.ML.FastTree/FastTree.cs:1361 in 08c9001. [](commit_id = 08c9001, deletion_comment = False)

Ivanidzo4ka

TomFinley · 2018-06-28T20:36:01Z

                    groupInfo = ColumnInfo.CreateFromName(data.Schema, convCol.Name, "converted group id");

Yes, I should definitely get rid of it -- it's not actually used for anything.

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

Refers to: src/Microsoft.ML.FastTree/FastTree.cs:1361 in 08c9001. [](commit_id = 08c9001, deletion_comment = False)

zeahmed · 2018-06-28T21:39:55Z

src/Microsoft.ML.Data/Commands/TrainCommand.cs

-                    view = CompositeDataLoader.Create(env, loader,
-                        new KeyValuePair<string, SubComponent<IDataTransform, SignatureDataTransform>>(null, component));
-                }
+                IDataView ApplyNormalizer(IHostEnvironment innerEnv, IDataView input)


If my PR goes in first, you will be able to use convenience constructor here...:) #Closed

I know, I even had a comment about that. :) But Ivan made me remove it. #Closed

zeahmed · 2018-06-28T21:45:47Z

I have bunch of changes in NormalizeColumn.cs which is being moved to different location in this PR. The best way to avoid extra work for me would be to let PR #405 go first and then this one.

What do you say? Can you hold this PR until #405 is merged? #Closed

zeahmed · 2018-06-28T21:53:15Z

src/Microsoft.ML.Core/Data/RoleMappedSchema.cs

+    /// This contains information about a column in an <see cref="IDataView"/>. It is essentially a convenience cache
+    /// containing the name, column index, and column type for the column. The intended usage is that users of <see cref="RoleMappedSchema"/>
+    /// will have a convenient method of getting the index and type without having to separately query it through the <see cref="ISchema"/>,
+    /// since practically the first thing a consumer of a <see cref="RoleMappedSchema"/> will want to do once they get a mappping is


I feel comment/summary is not complete here as sentence is ending in is? #Resolved

Sure. You go first then I'll adjust. This work in some ways will be helped by what you're doing as well, as you have noted. #Closed

Hmmm I do that sometimes don't I.

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

zeahmed · 2018-06-28T22:00:17Z

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

+        /// This method will not modify <paramref name="data"/> if the return from that is <c>null</c> or
+        /// <c>false</c>.</param>
+        /// <returns>True if the normalizer was applied and <paramref name="data"/> was modified</returns>
+        public static bool CreateIfNeeded(IHostEnvironment env, ref RoleMappedData data, ITrainer trainer)


Great! I will be using this method in #424 just before calling train method, correct? #Closed

I think so. Also probably before the hypothetical cache-if-needed. #Closed

zeahmed

eerhardt · 2018-07-02T19:13:17Z

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

+        {
+            // REVIEW: The role mapped data has the ability to have multiple columns fill the role of features, which is
+            // useful in some trainers that are nonetheless parameteric and can therefore benefit from normalization.
+            var featInfo = schema.Feature;


(nit) check schema for null? #Resolved

eerhardt · 2018-07-02T19:19:39Z

src/Microsoft.ML.Core/Data/MetadataUtils.cs

        }

+        /// <summary>
+        /// Returns whether a column has the <see cref="Kinds.IsNormalized"/> metadata set to true.


(minor) Do we instead want to document what IsNormalized means? "Returns whether a column <insert what IsNormalized means>". #Resolved

Sure, I can add a bit more information. Though that information, I might prefer to make that part of the Kinds static class, since the documentation of those are the primary source of truth. This is just something we added as a convenience on top of that.

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

eerhardt

LGTM. Just some minor questions.

* API conveniences for the Normalize transform

TomFinley requested review from Ivanidzo4ka, eerhardt, glebuk and zeahmed June 28, 2018 17:52

TomFinley commented Jun 28, 2018

View reviewed changes

Ivanidzo4ka reviewed Jun 28, 2018

View reviewed changes

Ivanidzo4ka approved these changes Jun 28, 2018

View reviewed changes

zeahmed reviewed Jun 28, 2018

View reviewed changes

TomFinley changed the title ~~[WIP] Normalization API helpers~~ Normalization API helpers Jul 2, 2018

zeahmed approved these changes Jul 2, 2018

View reviewed changes

eerhardt reviewed Jul 2, 2018

View reviewed changes

eerhardt approved these changes Jul 2, 2018

View reviewed changes

Tom Finley and others added 4 commits July 2, 2018 14:08

API conveniences for the Normalize transform

6c9bc0e

Minor changes

f446afa

Ivan comments

7fb15ae

Use normalize convenience constructors.

3f1e454

TomFinley force-pushed the tfinley/NormalHelpers branch from d9e0b4d to 3f1e454 Compare July 2, 2018 21:23

PR comments.

5153afb

TomFinley merged commit 53c2a15 into dotnet:master Jul 3, 2018

TomFinley deleted the tfinley/NormalHelpers branch July 5, 2018 20:07

shauheen mentioned this pull request Jul 17, 2018

Direct API: Auto-normalization #433

Closed

eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018

Normalization API helpers (dotnet#446)

212b9b1

* API conveniences for the Normalize transform

TomFinley mentioned this pull request Aug 16, 2018

Replace SubComponent with IComponentFactory in ML.Ensemble #681

Merged

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

Conversation

TomFinley commented Jun 28, 2018

Uh oh!

TomFinley Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jun 28, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka commented Jun 28, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jun 28, 2018

Uh oh!

zeahmed Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeahmed Jun 28, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

zeahmed Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 2, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jul 2, 2018 • edited by TomFinley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TomFinley Jun 28, 2018 •

edited

Loading

Ivanidzo4ka Jun 28, 2018 •

edited by TomFinley

Loading

Ivanidzo4ka commented Jun 28, 2018 •

edited by TomFinley

Loading

zeahmed Jun 28, 2018 •

edited

Loading

TomFinley Jun 28, 2018 •

edited

Loading

zeahmed commented Jun 28, 2018 •

edited

Loading

zeahmed Jun 28, 2018 •

edited by TomFinley

Loading

TomFinley Jun 28, 2018 •

edited

Loading

zeahmed Jun 28, 2018 •

edited

Loading

TomFinley Jun 28, 2018 •

edited

Loading

eerhardt Jul 2, 2018 •

edited by TomFinley

Loading

eerhardt Jul 2, 2018 •

edited by TomFinley

Loading