Scrubbing PkPd #2749

singlis · 2019-02-26T23:08:22Z

Renamed pkpd class to PkPdTrainer
Renamed pkpd.cs to PkPdTrainer.cs
Updates to PairwiseCoupling api such as removing abbreviations from parameter names, updating comments and documentation

Ivanidzo4ka · 2019-02-26T23:09:54Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

@@ -623,7 +623,7 @@ private static ICalibratorTrainer GetCalibratorTrainerOrThrow(IExceptionContext
        /// <param name="imputeMissingLabelsAsNegative">Whether to treat missing labels as having negative labels, instead of keeping them missing.</param>
        /// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>
        /// <typeparam name="TModel">The type of the model. This type parameter will usually be inferred automatically from <paramref name="binaryEstimator"/>.</typeparam>
-        public static Pkpd PairwiseCoupling<TModel>(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog,
+        public static PkpdTrainer PairwiseCoupling<TModel>(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog,


PkpdTrainer PairwiseCoupling [](start = 22, length = 28)

PairwiseCouplingTrainer PairwiseCoupling Let's match name of trainer and method in catalog #Resolved

Ok thanks, I renamed pkpd to PairwiseCouplingTrainer

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

Ivanidzo4ka · 2019-02-26T23:18:37Z

    private readonly TDistPredictor[] _predictors;

In Ova I can access internal predictors, in Pkpd, I can't.
Feel somewhat inconsistent.
Not sure what to do with them, so my default option would be to hide them in both places and open later if needed.
#Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Pkpd.cs:245 in af544af. [](commit_id = af544af, deletion_comment = False)

Ivanidzo4ka · 2019-02-26T23:19:35Z

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

@@ -42,17 +42,17 @@ namespace Microsoft.ML.Trainers
    /// pair.
    ///
    /// These two can allow you to exploit trainers that do not naturally have a
-    /// multiclass option, for example, using the Runtime.FastTree.FastTreeBinaryClassificationTrainer
+    /// multiclass option, for example, using the Microsoft.ML.Trainers.FastTree.FastTreeBinaryClassificationTrainer


Microsoft.ML.Trainers.FastTree.FastTreeBinaryClassificationTrainer [](start = 50, length = 66)

use <see cref="something"/> otherwise it become obsolete again. #WontFix

I originally tried this, however it would require the StandardLearners to take a dependency on FastTree. Not worth it for documentation.

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

I have another solution, will post here soon.

In reply to: 260540515 [](ancestors = 260540515,260532822)

najeeb-kazmi · 2019-02-26T23:47:54Z

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

    /// to solve a multiclass problem.
    /// Alternately, it can allow ML.NET to solve a "simpler" problem even in the cases
    /// where the trainer has a multiclass option, but using it directly is not
-    /// practical due to, usually, memory constraints.For example, while a multiclass
+    /// practical due to, usually, memory constraints. For example, while a multiclass
    /// 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.


nit: If we are renaming OVA to OneVsAll, I'd also change it here. #Resolved

ah! Thanks, I will update.

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

singlis · 2019-02-26T23:54:49Z

    private readonly TDistPredictor[] _predictors;

I agree -- I will make the predictors private in my ova changes and see if anything breaks.

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/MultiClass/Pkpd.cs:245 in af544af. [](commit_id = af544af, deletion_comment = False)

codecov · 2019-02-26T23:59:44Z

Codecov Report

Merging #2749 into master will decrease coverage by 0.01%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #2749      +/-   ##
==========================================
- Coverage    71.7%   71.68%   -0.02%     
==========================================
  Files         809      809              
  Lines      142401   142402       +1     
  Branches    16112    16112              
==========================================
- Hits       102102   102083      -19     
- Misses      35864    35888      +24     
+ Partials     4435     4431       -4

Flag	Coverage Δ
#Debug	`71.68% <80%> (-0.02%)`	⬇️
#production	`67.93% <80%> (-0.02%)`	⬇️
#test	`85.86% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
...ML.Tests/TrainerEstimators/MetalinearEstimators.cs	`100% <ø> (ø)`	⬆️
...oft.ML.StandardLearners/StandardLearnersCatalog.cs	`88.63% <100%> (+0.06%)`	⬆️
...ers/Standard/MultiClass/PairwiseCouplingTrainer.cs	`88.23% <75%> (ø)`
src/Microsoft.ML.Core/Data/ProgressReporter.cs	`70.95% <0%> (-6.99%)`	⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs	`82.66% <0%> (-0.21%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`85.53% <0%> (-0.17%)`	⬇️
...StandardLearners/Standard/LinearModelParameters.cs	`60.9% <0%> (+0.26%)`	⬆️

artidoro · 2019-02-27T17:39:53Z

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

@@ -213,6 +214,9 @@ public override TTransformer Fit(IDataView input)
        }
    }

+    /// <summary>
+    /// Contains the model parameters and prediction functions for Pkpd.
+    /// </summary>
    public sealed class PkpdModelParameters :


PkpdModelParameters [](start = 24, length = 19)

Maybe rename this one too, so that it does not use the contraction. #Resolved

Also could you rename Pkpd in the summary above?

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

artidoro · 2019-02-27T17:40:08Z

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

@@ -167,7 +168,7 @@ private IDataView MapLabels(RoleMappedData data, int cls1, int cls2)
                return MapLabelsCore(NumberDataViewType.Double, (in double val) => val == key1 || val == key2, data);
            }

-            throw Host.ExceptNotSupp($"Label column type is not supported by PKPD: {lab.Type}");
+            throw Host.ExceptNotSupp($"Label column type is not supported by PkPdTrainer: {lab.Type.RawType}");


PkPdTrainer [](start = 77, length = 11)

Can you rename this too? #Resolved

Thanks!

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

artidoro

Two minor comments otherwise looks good!

- Renamed pkpd.cs to PairwiseCouplingTrainer.cs - Updates to PairwiseCoupling api such as removing abbreviations from parameter names, updating comments and documentation. This closes dotnet#2619

sfilipi · 2019-03-01T18:48:54Z

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

@@ -167,7 +166,7 @@ private IDataView MapLabels(RoleMappedData data, int cls1, int cls2)
                return MapLabelsCore(NumberDataViewType.Double, (in double val) => val == key1 || val == key2, data);
            }

-            throw Host.ExceptNotSupp($"Label column type is not supported by PKPD: {lab.Type}");
+            throw Host.ExceptNotSupp($"Label column type is not supported by PairwiseCouplingTrainer: {lab.Type.RawType}");


PairwiseCouplingTrainer [](start = 77, length = 23)

nameof

sfilipi

singlis requested review from Ivanidzo4ka, artidoro and sfilipi February 26, 2019 23:08

Ivanidzo4ka reviewed Feb 26, 2019

View reviewed changes

najeeb-kazmi reviewed Feb 26, 2019

View reviewed changes

artidoro reviewed Feb 27, 2019

View reviewed changes

artidoro approved these changes Feb 27, 2019

View reviewed changes

singlis force-pushed the singlis/pkpd branch from b63a285 to bafd2fe Compare March 1, 2019 18:45

sfilipi reviewed Mar 1, 2019

View reviewed changes

sfilipi approved these changes Mar 1, 2019

View reviewed changes

singlis added 2 commits March 1, 2019 13:23

Merge remote-tracking branch 'upstream/master' into singlis/pkpd

89c2934

- Updating based on feedback

100126c

singlis self-assigned this Mar 1, 2019

singlis merged commit 49a96c5 into dotnet:master Mar 1, 2019

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

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

Scrubbing PkPd #2749

Scrubbing PkPd #2749

Conversation

singlis commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka Feb 26, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka commented Feb 26, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka Feb 26, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

singlis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

najeeb-kazmi Feb 26, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

singlis commented Feb 26, 2019

Uh oh!

codecov bot commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

artidoro Feb 27, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artidoro Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

artidoro Feb 27, 2019 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

artidoro left a comment

Choose a reason for hiding this comment

Uh oh!

sfilipi Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

sfilipi left a comment

Choose a reason for hiding this comment

Uh oh!

singlis commented Feb 26, 2019 •

edited

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by singlis

Loading

Ivanidzo4ka commented Feb 26, 2019 •

edited by singlis

Loading

Ivanidzo4ka Feb 26, 2019 •

edited by singlis

Loading

najeeb-kazmi Feb 26, 2019 •

edited by singlis

Loading

codecov bot commented Feb 26, 2019 •

edited

Loading

artidoro Feb 27, 2019 •

edited by singlis

Loading

artidoro Feb 27, 2019 •

edited by singlis

Loading