Fixing parameters in ML.NET Public API #2665

abgoswam · 2019-02-20T19:41:04Z

Also related to #2680 #2613

Using ApiReview tool, went through the public surface area making the changes (as described in the issue)

codecov · 2019-02-20T20:27:29Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2665   +/-   ##
=========================================
  Coverage          ?   71.65%           
=========================================
  Files             ?      804           
  Lines             ?   141872           
  Branches          ?    16111           
=========================================
  Hits              ?   101654           
  Misses            ?    35781           
  Partials          ?     4437

Flag	Coverage Δ
#Debug	`71.65% <97.67%> (?)`
#production	`67.95% <97.05%> (?)`
#test	`85.74% <100%> (?)`

Impacted Files	Coverage Δ
...actorizationMachine/FactorizationMachineCatalog.cs	`50% <0%> (ø)`
...oft.ML.StandardLearners/StandardLearnersCatalog.cs	`88.57% <100%> (ø)`
src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs	`88.37% <100%> (ø)`
src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs	`94.28% <100%> (ø)`
test/Microsoft.ML.Tests/OnnxConversionTest.cs	`97.16% <100%> (ø)`
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`100% <100%> (ø)`
src/Microsoft.ML.LightGBM/LightGbmCatalog.cs	`87.5% <100%> (ø)`
...s/Api/CookbookSamples/CookbookSamplesDynamicApi.cs	`80.4% <100%> (ø)`
src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs	`100% <100%> (ø)`
src/Microsoft.ML.PCA/PCACatalog.cs	`53.84% <100%> (ø)`
... and 2 more

TomFinley · 2019-02-21T04:33:00Z

src/Microsoft.ML.KMeansClustering/KMeansCatalog.cs

@@ -29,7 +29,7 @@ public static class KMeansClusteringExtensions
        /// </example>
        public static KMeansPlusPlusTrainer KMeans(this ClusteringCatalog.ClusteringTrainers catalog,
           string featureColumn = DefaultColumnNames.Features,


featureColumn [](start = 18, length = 13)

So we're calling this exampleWeightColumnName, but calling the above featureColumn. I'm not terribly insistent one way or the other, but we should at least be consistent I think.

In the original issue you proposed calling this * exampleWeightColumn, which seems to me to be consistent with the existing behavior here. Yet elsewhere I see that we are calling things labelColumn and labelColumnName here and there.

Is this on someone's radar? If we are not going to fix it (we should!) a second best alternative would be to be at least locally consistent. Not sure what @rogancarr and @eerhardt and @sfilipi think? #Resolved

Issue #2177 proposes we use suffix ColumnName consistently

In this PR I am making that change for weight, groupid parameters. In a follow up PR I will do it for label and features.

So once both PRs go in, we will mark #2177 as complete, and the following convention will be used

labelColumnName

featureColumnName

exampleWeightColumnName

rowGroupColumnName

This is certainly on my radar, as we drive towards consistency in the APIs . I have also updated the description of the issue so it has reference to issue #2177 . Hope that sounds good ? #Closed

Very good, thank you @abgoswam ! Sorry missed that discussion. #Resolved

@TomFinley. I have consolidated all the changes in this PR. So this PR addresses {features, labels, groupid, weight} column names in a consistent manner.

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

@TomFinley. I have consolidated all the changes in this PR. So this PR addresses {features, labels, groupid, weight} column names in a consistent manner.

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

TomFinley · 2019-02-21T04:52:58Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

        {
            Contracts.CheckValue(catalog, nameof(catalog));
            var env = CatalogUtils.GetEnvironment(catalog);
            var options = new OlsLinearRegressionTrainer.Options
            {
                LabelColumn = labelColumnName,
                FeatureColumn = featureColumnName,
-                WeightColumn = weightColumn != null ? Optional<string>.Explicit(weightColumn) : Optional<string>.Implicit(DefaultColumnNames.Weight)
+                WeightColumn = exampleWeightColumnName != null ? Optional<string>.Explicit(exampleWeightColumnName) : Optional<string>.Implicit(DefaultColumnNames.Weight)


Uh oh. Am I wrong or is there literally no way to have non-weighted training if you happen to have a column named Weight????

It looks like there's an existing issue #1065 that is capturing this. Not your code, so don't have to fix, I just notice this way. #Resolved

Although I see it's incidentally assigned to you @abgoswam . 😄 Maybe after this PR we can think about how to address this. #Resolved

Sounds good. We should fix this behavior as part of #1065 (in a separate PR). #Closed

eerhardt · 2019-02-21T17:12:54Z

src/Microsoft.ML.PCA/PCACatalog.cs

        /// <param name="rank">The number of principal components.</param>
        /// <param name="overSampling">Oversampling parameter for randomized PrincipalComponentAnalysis training.</param>
        /// <param name="center">If enabled, data is centered to be zero mean.</param>
        /// <param name="seed">The seed for random number generation.</param>
        public static PrincipalComponentAnalysisEstimator ProjectToPrincipalComponents(this TransformsCatalog.ProjectionTransforms catalog,
            string outputColumnName,
            string inputColumnName = null,
-            string weightColumn = PrincipalComponentAnalysisEstimator.Defaults.WeightColumn,
+            string exampleWeightColumnName = PrincipalComponentAnalysisEstimator.Defaults.WeightColumn,


Defaulting this to null will be covered in #1065, right? #Resolved

Yes, perhaps #1065 would be the right place to set this to null as default value, since that issue related to changing the behavior of the weights parameters.

The issues addressed by this PR are all related to consistency in API naming convention. #Resolved

#1065 did not address this. hence taking care of this in this PR

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

fixed in this PR itself.

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

eerhardt · 2019-02-24T15:33:16Z

src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

-        /// <param name="weights">The optional weights column.</param>
+        /// <param name="labelColumnName">The name of the label column.</param>
+        /// <param name="featureColumnName">The name of the feature column.</param>
+        /// <param name="rowGroupColumnName">The name of the group column.</param>


This isn't necessary for this PR, but I wonder if we need some documentation around the row group column. I'm obviously not an expert, but I have no idea what this is.

fyi...#2536 gives some context on this. we can add more documentation around this in follow up documentation focused PRs.

eerhardt · 2019-02-24T15:35:12Z

test/Microsoft.ML.Tests/Scenarios/Api/CookbookSamples/CookbookSamplesDynamicApi.cs

@@ -513,7 +513,7 @@ private static void RunEndToEnd(MLContext mlContext, IDataView trainData, string
            // Construct the learning pipeline. Note that we are now providing a contract name for the custom mapping:
            // otherwise we will not be able to save the model.
            var estimator = mlContext.Transforms.CustomMapping<InputRow, OutputRow>(CustomMappings.IncomeMapping, nameof(CustomMappings.IncomeMapping))
-                .Append(mlContext.BinaryClassification.Trainers.FastTree(labelColumn: "Label"));
+                .Append(mlContext.BinaryClassification.Trainers.FastTree(labelColumnName: "Label"));


This usually means the cookbook .md file needs to be updated as well. #Resolved

Thanks for making me take a second look at this. Updated the cookbook .md file in a couple of places.

p.s. my initial search for "labelColumn" in the cookbook .md file did not return relevant results. Apparently it used to be called just "label" at some point :)

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

eerhardt

Ivanidzo4ka

local tests run fine

9ecb0d5

abgoswam requested review from sfilipi, artidoro and Ivanidzo4ka February 20, 2019 19:41

abgoswam changed the title ~~Fixing weights parameter in ML.NET Public API~~ [WIP] Fixing (weights , groupid) parameters in ML.NET Public API Feb 20, 2019

TomFinley reviewed Feb 21, 2019

View reviewed changes

TomFinley mentioned this pull request Feb 21, 2019

WeightColumn name defaults to "Weights" in one of the constructors of the estimators #1065

Closed

TomFinley reviewed Feb 21, 2019

View reviewed changes

eerhardt reviewed Feb 21, 2019

View reviewed changes

Ivanidzo4ka mentioned this pull request Feb 22, 2019

Added samples & docs for BinaryClassification.StochasticGradientDescent #2688

Merged

abgoswam added 2 commits February 23, 2019 19:17

merge with master

a3b5b16

made fixes for (feature, label) as well

8724f55

abgoswam changed the title ~~[WIP] Fixing (weights , groupid) parameters in ML.NET Public API~~ Fixing parameters in ML.NET Public API Feb 23, 2019

abgoswam requested review from eerhardt and TomFinley February 23, 2019 20:52

eerhardt reviewed Feb 24, 2019

View reviewed changes

eerhardt approved these changes Feb 24, 2019

View reviewed changes

update cookbook md file

f785167

Ivanidzo4ka approved these changes Feb 25, 2019

View reviewed changes

abgoswam merged commit 7cc208c into dotnet:master Feb 25, 2019

This was referenced Feb 25, 2019

Rename GroupId column to RowGroup #2660

Closed

The parameter descriptions should distinguish between columns and column names. #2177

Closed

abgoswam deleted the abgoswam/parameter_consistency branch March 20, 2019 20:13

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

Fixing parameters in ML.NET Public API #2665

Fixing parameters in ML.NET Public API #2665

abgoswam commented Feb 20, 2019 •

edited

Loading

codecov bot commented Feb 20, 2019 •

edited

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 23, 2019

abgoswam Feb 23, 2019

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

eerhardt Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

abgoswam Feb 23, 2019

abgoswam Feb 23, 2019

eerhardt Feb 24, 2019

abgoswam Feb 24, 2019

eerhardt Feb 24, 2019 •

edited by abgoswam

Loading

abgoswam Feb 24, 2019

eerhardt left a comment

Ivanidzo4ka left a comment

Fixing parameters in ML.NET Public API #2665

Fixing parameters in ML.NET Public API #2665

Conversation

abgoswam commented Feb 20, 2019 • edited Loading

codecov bot commented Feb 20, 2019 • edited Loading

Codecov Report

TomFinley Feb 21, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Feb 21, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 23, 2019

Choose a reason for hiding this comment

abgoswam Feb 23, 2019

Choose a reason for hiding this comment

TomFinley Feb 21, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

TomFinley Feb 21, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

eerhardt Feb 21, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

abgoswam Feb 23, 2019

Choose a reason for hiding this comment

abgoswam Feb 23, 2019

Choose a reason for hiding this comment

eerhardt Feb 24, 2019

Choose a reason for hiding this comment

abgoswam Feb 24, 2019

Choose a reason for hiding this comment

eerhardt Feb 24, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 24, 2019

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

abgoswam commented Feb 20, 2019 •

edited

Loading

codecov bot commented Feb 20, 2019 •

edited

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

TomFinley Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

eerhardt Feb 21, 2019 •

edited by abgoswam

Loading

abgoswam Feb 21, 2019 •

edited

Loading

eerhardt Feb 24, 2019 •

edited by abgoswam

Loading