Creation of components through MLContext and cleanup (Convert, DropSlots, FeatureSelection) #2365

artidoro · 2019-02-01T09:20:28Z

This PR is part of the work outlined in #1798, and focuses on the Convert, DropSlots, CountFeatureSelection, MutualInformationFeatureSelection, FeatureContributionCalculationTransform, PermutationFeatureImportanceTransform, MissingValueDropping, TreeEnsembleFeaturizationTransform 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

codecov · 2019-02-01T09:58:26Z

Codecov Report

Merging #2365 into master will decrease coverage by <.01%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##           master    #2365      +/-   ##
==========================================
- Coverage   71.23%   71.22%   -0.01%     
==========================================
  Files         785      785              
  Lines      141034   141030       -4     
  Branches    16114    16116       +2     
==========================================
- Hits       100468   100454      -14     
- Misses      36095    36106      +11     
+ Partials     4471     4470       -1

Flag	Coverage Δ
#Debug	`71.22% <73.68%> (-0.01%)`	⬇️
#production	`67.57% <62.12%> (-0.02%)`	⬇️
#test	`85.31% <100%> (ø)`	⬆️

Ivanidzo4ka · 2019-02-01T18:18:08Z

    /// <param name="inputColumnName">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>        /// <param name="hashBits">Number of bits to hash into. Must be between 1 and 31, inclusive.</param>

can you fix it? #Closed

Refers to: src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs:25 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

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

Ivanidzo4ka · 2019-02-01T18:22:21Z

    public readonly bool Normalize;

can we comment them or hide from user? #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/FeatureContributionCalculationTransform.cs:104 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

Ivanidzo4ka · 2019-02-01T18:23:35Z

/// ConvertTransform allow to change underlying column type as long as we know how to convert types.

can you use #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/TypeConverting.cs:56 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

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

Ivanidzo4ka · 2019-02-01T18:25:07Z

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

+        /// <summary>
+        /// Returns the <see cref="SchemaShape"/> of the schema which will be produced by the transformer.
+        /// Used for schema propagation and verification in a pipeline.
+        /// </summary>


/// ? #Closed

Please disregard my comments, apparently it doesn't work for intellisense. dotnet/csharplang#313

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

Yes we were talking about this with senja yesterday! it would be super useful!

In reply to: 253154622 [](ancestors = 253154622,253150851)

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

Ivanidzo4ka

Ivanidzo4ka · 2019-02-01T21:22:13Z

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

@@ -78,8 +78,8 @@ public void TestConvertWorkout()
            var data = new[] { new TestClass() { A = 1, B = new int[2] { 1,4 } },
                               new TestClass() { A = 2, B = new int[2] { 3,4 } }};
            var dataView = ML.Data.ReadFromEnumerable(data);
-            var pipe = new TypeConvertingEstimator(Env, columns: new[] {new TypeConvertingTransformer.ColumnInfo("ConvA", DataKind.R4, "A"),
-                new TypeConvertingTransformer.ColumnInfo("ConvB", DataKind.R4, "B")});
+            var pipe = new TypeConvertingEstimator(Env, columns: new[] {new TypeConvertingEstimator.ColumnInfo("ConvA", DataKind.R4, "A"),


TypeConvertingEstimator [](start = 27, length = 23)

TypeConvertingEstimator [](start = 27, length = 23)

why not ml.Catalog.ConvertType? #Closed

Sure! Let's do it :)

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

artidoro · 2019-02-01T22:07:31Z

    /// <param name="inputColumnName">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>        /// <param name="hashBits">Number of bits to hash into. Must be between 1 and 31, inclusive.</param>

Sorry I don't see what's wrong.

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

Refers to: src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs:25 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

Ivanidzo4ka · 2019-02-01T22:09:21Z

    /// <param name="inputColumnName">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>        /// <param name="hashBits">Number of bits to hash into. Must be between 1 and 31, inclusive.</param>

It's a two <param> in one line. I'm just asking to press enter :)

In reply to: 459884561 [](ancestors = 459884561,459817377)

Refers to: src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs:25 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

artidoro · 2019-02-01T22:10:15Z

    /// <param name="inputColumnName">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>        /// <param name="hashBits">Number of bits to hash into. Must be between 1 and 31, inclusive.</param>

Ah I see! I did not see the rest of the line! :)

In reply to: 459885033 [](ancestors = 459885033,459884561,459817377)

Refers to: src/Microsoft.ML.Data/Transforms/ConversionsExtensionsCatalog.cs:25 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

artidoro · 2019-02-01T22:12:33Z

    public readonly bool Normalize;

Ok, I hid them.

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

Refers to: src/Microsoft.ML.Data/Transforms/FeatureContributionCalculationTransform.cs:104 in bd98b16. [](commit_id = bd98b16, deletion_comment = False)

abgoswam · 2019-02-05T19:23:48Z

src/Microsoft.ML.EntryPoints/FeatureCombiner.cs

@@ -312,7 +312,7 @@ public static CommonOutputs.TransformOutput PrepareRegressionLabel(IHostEnvironm
                    }
                }
            };


don't see this being used . can this be deleted ? #Resolved

Yes good point!

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

abgoswam · 2019-02-05T19:24:54Z

src/Microsoft.ML.Transforms/CountFeatureSelection.cs

@@ -46,6 +46,9 @@ public sealed class Arguments : TransformInputBase

        internal static string RegistrationName = "CountFeatureSelectionTransform";

+        /// <summary>


move to the corresponding Transformer ? #Resolved

We are moving them to the estimators right? I think it should stay here

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

sfilipi · 2019-02-06T18:18:36Z

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

@@ -168,37 +171,11 @@ private static VersionInfo GetVersionInfo()

        private const string RegistrationName = "Convert";

-        public IReadOnlyCollection<ColumnInfo> Columns => _columns.AsReadOnly();
+        public IReadOnlyCollection<TypeConvertingEstimator.ColumnInfo> Columns => _columns.AsReadOnly();


Columns [](start = 71, length = 7)

xml #Resolved

sfilipi · 2019-02-06T18:20:44Z

        public readonly long MinCount;

xml #Resolved

Refers to: src/Microsoft.ML.Transforms/CountFeatureSelection.cs:56 in 72e4b49. [](commit_id = 72e4b49, deletion_comment = False)

sfilipi

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

artidoro self-assigned this Feb 1, 2019

artidoro requested review from sfilipi, najeeb-kazmi, Ivanidzo4ka, singlis, zeahmed, wschin and abgoswam February 1, 2019 09:21

artidoro mentioned this pull request Feb 1, 2019

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

Closed

Ivanidzo4ka reviewed Feb 1, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/FeatureContributionCalculationTransformer.cs Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 1, 2019

View reviewed changes

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

Ivanidzo4ka reviewed Feb 1, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/TypeConverting.cs Show resolved Hide resolved

Ivanidzo4ka approved these changes Feb 1, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 1, 2019

View reviewed changes

abgoswam reviewed Feb 5, 2019

View reviewed changes

artidoro force-pushed the round3 branch from 8e19654 to ec34bb5 Compare February 6, 2019 00:41

sfilipi reviewed Feb 6, 2019

View reviewed changes

sfilipi approved these changes Feb 6, 2019

View reviewed changes

artidoro added 5 commits February 6, 2019 21:22

feature selection transforms mainly

5ffcf7b

review comments and some more cleanup

4e6e5db

review comments

af56964

resolved review comments

cd9fdbb

entrypoint catalog

dd5688f

artidoro force-pushed the round3 branch from 72e4b49 to dd5688f Compare February 7, 2019 05:34

artidoro merged commit b2851e5 into dotnet:master Feb 7, 2019

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creation of components through MLContext and cleanup (Convert, DropSlots, FeatureSelection) #2365

Creation of components through MLContext and cleanup (Convert, DropSlots, FeatureSelection) #2365

artidoro commented Feb 1, 2019

codecov bot commented Feb 1, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

artidoro Feb 1, 2019

Ivanidzo4ka left a comment

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

artidoro Feb 1, 2019

artidoro commented Feb 1, 2019

Ivanidzo4ka commented Feb 1, 2019

artidoro commented Feb 1, 2019

artidoro commented Feb 1, 2019

abgoswam Feb 5, 2019 •

edited by artidoro

Loading

artidoro Feb 6, 2019

abgoswam Feb 5, 2019 •

edited by artidoro

Loading

artidoro Feb 6, 2019

sfilipi Feb 6, 2019 •

edited by artidoro

Loading

sfilipi commented Feb 6, 2019 •

edited by artidoro

Loading

sfilipi left a comment

		@@ -46,6 +46,9 @@ public sealed class Arguments : TransformInputBase

		internal static string RegistrationName = "CountFeatureSelectionTransform";

		/// <summary>

Creation of components through MLContext and cleanup (Convert, DropSlots, FeatureSelection) #2365

Creation of components through MLContext and cleanup (Convert, DropSlots, FeatureSelection) #2365

Conversation

artidoro commented Feb 1, 2019

codecov bot commented Feb 1, 2019 • edited Loading

Codecov Report

Ivanidzo4ka commented Feb 1, 2019 • edited Loading

Ivanidzo4ka commented Feb 1, 2019 • edited by artidoro Loading

Ivanidzo4ka commented Feb 1, 2019 • edited by artidoro Loading

Ivanidzo4ka Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

artidoro Feb 1, 2019

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

artidoro Feb 1, 2019

Choose a reason for hiding this comment

artidoro commented Feb 1, 2019

Ivanidzo4ka commented Feb 1, 2019

artidoro commented Feb 1, 2019

artidoro commented Feb 1, 2019

abgoswam Feb 5, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

artidoro Feb 6, 2019

Choose a reason for hiding this comment

abgoswam Feb 5, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

artidoro Feb 6, 2019

Choose a reason for hiding this comment

sfilipi Feb 6, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

sfilipi commented Feb 6, 2019 • edited by artidoro Loading

sfilipi left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 1, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 1, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

Ivanidzo4ka Feb 1, 2019 •

edited

Loading

abgoswam Feb 5, 2019 •

edited by artidoro

Loading

abgoswam Feb 5, 2019 •

edited by artidoro

Loading

sfilipi Feb 6, 2019 •

edited by artidoro

Loading

sfilipi commented Feb 6, 2019 •

edited by artidoro

Loading