Scrubbing OneVsAll #2743

singlis · 2019-02-26T19:07:38Z

Renamed OVA to OneVsAllTrainer
Updates to comments and documentation
Related to Scrubbing Meta learners #2619

codecov · 2019-02-26T20:00:14Z

Codecov Report

Merging #2743 into master will decrease coverage by <.01%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #2743      +/-   ##
==========================================
- Coverage   71.67%   71.67%   -0.01%     
==========================================
  Files         809      809              
  Lines      142416   142416              
  Branches    16120    16120              
==========================================
- Hits       102077   102073       -4     
- Misses      35909    35911       +2     
- Partials     4430     4432       +2

Flag	Coverage Δ
#Debug	`71.67% <70%> (-0.01%)`	⬇️
#production	`67.91% <76.47%> (-0.01%)`	⬇️
#test	`85.86% <33.33%> (ø)`	⬆️

Impacted Files	Coverage Δ
...ft.ML.StandardLearners/Standard/MultiClass/Pkpd.cs	`88.23% <ø> (ø)`	⬆️
...rners/Standard/MultiClass/MetaMulticlassTrainer.cs	`86.31% <ø> (ø)`	⬆️
...ML.LightGBM.StaticPipe/LightGbmStaticExtensions.cs	`46.42% <ø> (ø)`	⬆️
test/Microsoft.ML.Benchmarks/Numeric/Ranking.cs	`0% <0%> (ø)`	⬆️
...oft.ML.Benchmarks/Text/MultiClassClassification.cs	`0% <0%> (ø)`	⬆️
...est/Microsoft.ML.StaticPipelineTesting/Training.cs	`99.25% <100%> (ø)`	⬆️
...Microsoft.ML.LightGBM/LightGbmMulticlassTrainer.cs	`87.94% <100%> (ø)`	⬆️
...oft.ML.StandardLearners/StandardLearnersCatalog.cs	`88.57% <100%> (ø)`	⬆️
src/Microsoft.ML.EntryPoints/ModelOperations.cs	`100% <100%> (ø)`	⬆️
...earners/Standard/MultiClass/OneVersusAllTrainer.cs	`74.17% <66.66%> (ø)`
... and 4 more

Ivanidzo4ka · 2019-02-26T20:42:20Z

        IEstimator<ISingleFeaturePredictionTransformer<ICalibrator>> calibrator = null,

@wschin do we have any plans to do something about this one? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs:597 in 24ecdcc. [](commit_id = 24ecdcc, deletion_comment = False)

artidoro · 2019-02-26T21:01:50Z

// The .NET Foundation licenses this file to you under the MIT license.

Could you also rename the file? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Ova.cs:2 in 24ecdcc. [](commit_id = 24ecdcc, deletion_comment = False)

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

singlis · 2019-02-26T21:31:59Z

// The .NET Foundation licenses this file to you under the MIT license.

yes.

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Ova.cs:2 in 24ecdcc. [](commit_id = 24ecdcc, deletion_comment = False)

wschin · 2019-02-27T00:52:38Z

IEstimator<ISingleFeaturePredictionTransformer> calibrator = null,

@wschin do we have any plans to do something about this one?

Refers to: src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs:597 in 24ecdcc. [](commit_id = 24ecdcc, deletion_comment = False)

This doesn't look very good to me but my assignment for fixing that is postponed. #Resolved

wschin · 2019-02-28T01:25:56Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/MetaMulticlassTrainer.cs

@@ -20,7 +20,7 @@ public abstract class MetaMulticlassTrainer<TTransformer, TModel> : ITrainerEsti
        where TTransformer : ISingleFeaturePredictionTransformer<TModel>
        where TModel : class
    {
-        public abstract class OptionsBase
+        internal abstract class OptionsBase


Why do we internalize an Options of a trainer? #Resolved

This is internal because the only class that derives from it, Options for pkpd and ova are both internal.

The option classes are internal because there are no advanced options and all of the arguments are exposed in the argument-based constructor.

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

wschin · 2019-02-28T01:28:59Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/OneVersusAllTrainer.cs

        /// </summary>
        /// <param name="env">The <see cref="IHostEnvironment"/> instance.</param>
        /// <param name="binaryEstimator">An instance of a binary <see cref="ITrainerEstimator{TTransformer, TPredictor}"/> used as the base trainer.</param>
-        /// <param name="calibrator">The calibrator. If a calibrator is not explicitely provided, it will default to <see cref="PlattCalibratorTrainer"/></param>
+        /// <param name="calibrator">The calibrator. If a calibrator is not provided, it will default to <see cref="PlattCalibratorTrainer"/></param>


How does a naturally-calibrated model (say, logistic regression) interact with this parameter? #Resolved

I dont know for sure - I will posted an issue as a question. #2805

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

Also I think it would be worth updating this comment once I have more info. I won't that block checking this in for now though.

In reply to: 261430492 [](ancestors = 261430492,261015949)

wschin · 2019-02-28T01:33:42Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/OneVersusAllTrainer.cs

        /// <param name="labelColumn">The name of the label colum.</param>
-        /// <param name="imputeMissingLabelsAsNegative">Whether to treat missing labels as having negative labels, instead of keeping them missing.</param>
+        /// <param name="imputeMissingLabelsAsNegative">If true will treat missing labels as negative labels.</param>
        /// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>


Suggested change

/// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>

/// <param name="maximumCalibrationExampleCount">Number of instances to train the calibrator.</param>

What's the indent scope of this PR? Inferred from its title, I assume that having better argument names is included. Please correct me if I am wrong. #Resolved

Better argument names is in scope for this PR.

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

wschin · 2019-02-28T01:33:55Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/OneVersusAllTrainer.cs

        /// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>
        /// <param name="useProbabilities">Use probabilities (vs. raw outputs) to identify top-score category.</param>
-        internal Ova(IHostEnvironment env,
+        internal OneVersusAllTrainer(IHostEnvironment env,
            TScalarTrainer binaryEstimator,
            string labelColumn = DefaultColumnNames.Label,


Suggested change

string labelColumn = DefaultColumnNames.Label,

string labelColumnName = DefaultColumnNames.Label,

``` #Resolved

wschin · 2019-02-28T01:34:17Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/OneVersusAllTrainer.cs

        /// </summary>
        /// <param name="env">The <see cref="IHostEnvironment"/> instance.</param>
        /// <param name="binaryEstimator">An instance of a binary <see cref="ITrainerEstimator{TTransformer, TPredictor}"/> used as the base trainer.</param>
-        /// <param name="calibrator">The calibrator. If a calibrator is not explicitely provided, it will default to <see cref="PlattCalibratorTrainer"/></param>
+        /// <param name="calibrator">The calibrator. If a calibrator is not provided, it will default to <see cref="PlattCalibratorTrainer"/></param>
        /// <param name="labelColumn">The name of the label colum.</param>


Suggested change

/// <param name="labelColumn">The name of the label colum.</param>

/// <param name="labelColumnName">The name of the label colum.</param>

``` #Resolved

wschin · 2019-02-28T01:38:42Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/Pkpd.cs

@@ -50,7 +50,7 @@ namespace Microsoft.ML.Trainers
    /// logistic regression is a more principled way to solve a multiclass problem, it
    /// requires that the learner store a lot more intermediate state in the form of
    /// L-BFGS history for all classes *simultaneously*, rather than just one-by-one
-    /// as would be needed for OVA.
+    /// as would be needed for OneVersusAll.


OneVersusAll [](start = 31, length = 12)

Please use see cref= if you mean a class or just one-versus-all classification model otherwise. #Resolved

wschin · 2019-02-28T01:40:01Z

        int maxCalibrationExamples = 1000000000,

maxCalibrationExampleCount #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Ova.cs:90 in 5093549. [](commit_id = 5093549, deletion_comment = False)

wschin · 2019-02-28T01:41:59Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/OneVersusAllTrainer.cs

@@ -49,7 +49,7 @@ public sealed class Ova : MetaMulticlassTrainer<MulticlassPredictionTransformer<
        private readonly Options _options;

        /// <summary>
-        /// Options passed to OVA.
+        /// Options passed to OneVsAll.


Please do see cref= here. #Resolved

singlis · 2019-02-28T22:32:32Z

        int maxCalibrationExamples = 1000000000,

good catch - I think this should be maximumCalibrationExampleCount

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Ova.cs:90 in 5093549. [](commit_id = 5093549, deletion_comment = False)

wschin

Overall LGTM. Btw, instead of min/max we use minimum/maximum in argument names.

singlis · 2019-03-01T00:55:41Z

build/Dependencies.props

@@ -15,7 +15,7 @@
    <GoogleProtobufPackageVersion>3.5.1</GoogleProtobufPackageVersion>
    <LightGBMPackageVersion>2.2.3</LightGBMPackageVersion>
    <MicrosoftMLOnnxRuntimePackageVersion>0.2.1</MicrosoftMLOnnxRuntimePackageVersion>
-    <MlNetMklDepsPackageVersion>0.0.0.7</MlNetMklDepsPackageVersion>
+    <MlNetMklDepsPackageVersion>0.0.0.8</MlNetMklDepsPackageVersion>


ugh! I merged a branch. Will revert these changes..

- Renamed OVA to OneVersusAllTrainer - Renames to abbreviated arguments - Updates to comments and documentation Related to dotnet#2619

singlis self-assigned this Feb 26, 2019

singlis requested review from Ivanidzo4ka, artidoro and sfilipi February 26, 2019 19:07

artidoro reviewed Feb 26, 2019

View reviewed changes

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs Show resolved Hide resolved

artidoro approved these changes Feb 26, 2019

View reviewed changes

wschin reviewed Feb 28, 2019

View reviewed changes

wschin approved these changes Feb 28, 2019

View reviewed changes

singlis commented Mar 1, 2019

View reviewed changes

Updates to OVA:

ef01610

- Renamed OVA to OneVersusAllTrainer - Renames to abbreviated arguments - Updates to comments and documentation Related to dotnet#2619

singlis force-pushed the singlis/ova branch from 5803c90 to ef01610 Compare March 1, 2019 01:35

singlis merged commit 2b417bb into dotnet:master Mar 1, 2019

singlis deleted the singlis/ova branch March 7, 2019 00:53

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

Scrubbing OneVsAll #2743

Scrubbing OneVsAll #2743

singlis commented Feb 26, 2019

codecov bot commented Feb 26, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by singlis

Loading

artidoro commented Feb 26, 2019 •

edited by singlis

Loading

singlis commented Feb 26, 2019

wschin commented Feb 27, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

singlis Feb 28, 2019

wschin Feb 28, 2019 •

edited by singlis

Loading

singlis Feb 28, 2019

singlis Feb 28, 2019

wschin Feb 28, 2019 •

edited by singlis

Loading

singlis Feb 28, 2019

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin commented Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

singlis commented Feb 28, 2019

wschin left a comment

singlis Mar 1, 2019

	/// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>
	/// <param name="maximumCalibrationExampleCount">Number of instances to train the calibrator.</param>

	string labelColumn = DefaultColumnNames.Label,
	string labelColumnName = DefaultColumnNames.Label,
	``` #Resolved

	/// <param name="labelColumn">The name of the label colum.</param>
	/// <param name="labelColumnName">The name of the label colum.</param>
	``` #Resolved

Scrubbing OneVsAll #2743

Scrubbing OneVsAll #2743

Conversation

singlis commented Feb 26, 2019

codecov bot commented Feb 26, 2019 • edited Loading

Codecov Report

Ivanidzo4ka commented Feb 26, 2019 • edited by singlis Loading

artidoro commented Feb 26, 2019 • edited by singlis Loading

singlis commented Feb 26, 2019

wschin commented Feb 27, 2019 • edited by singlis Loading

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Feb 28, 2019

Choose a reason for hiding this comment

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Feb 28, 2019

Choose a reason for hiding this comment

singlis Feb 28, 2019

Choose a reason for hiding this comment

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Feb 28, 2019

Choose a reason for hiding this comment

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

wschin commented Feb 28, 2019 • edited by singlis Loading

wschin Feb 28, 2019 • edited by singlis Loading

Choose a reason for hiding this comment

singlis commented Feb 28, 2019

wschin left a comment

Choose a reason for hiding this comment

singlis Mar 1, 2019

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by singlis

Loading

artidoro commented Feb 26, 2019 •

edited by singlis

Loading

wschin commented Feb 27, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading

wschin commented Feb 28, 2019 •

edited by singlis

Loading

wschin Feb 28, 2019 •

edited by singlis

Loading