Polish train catalog (renaming only) #3030

wschin · 2019-03-20T01:14:59Z

Related to #3029 (for StandardTrainersCatalog.cs) but for TrainCatalog.cs.

Rename topK to topPredictionCount
Rename k to falsePositiveCount

codecov · 2019-03-20T01:50:49Z

Codecov Report

Merging #3030 into master will increase coverage by <.01%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #3030      +/-   ##
==========================================
+ Coverage   72.48%   72.48%   +<.01%     
==========================================
  Files         804      804              
  Lines      144077   144078       +1     
  Branches    16179    16179              
==========================================
+ Hits       104434   104438       +4     
+ Misses      35228    35223       -5     
- Partials     4415     4417       +2

Flag	Coverage Δ
#Debug	`72.48% <90.9%> (ø)`	⬆️
#production	`68.12% <87.5%> (ø)`	⬆️
#test	`88.7% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
...crosoft.ML.StaticPipe/EvaluatorStaticExtensions.cs	`100% <100%> (ø)`	⬆️
test/Microsoft.ML.Tests/AnomalyDetectionTests.cs	`100% <100%> (ø)`	⬆️
...luators/Metrics/MulticlassClassificationMetrics.cs	`100% <100%> (ø)`	⬆️
test/Microsoft.ML.Tests/Scenarios/Api/TestApi.cs	`97.63% <100%> (ø)`	⬆️
...ML.Tests/Scenarios/IrisPlantClassificationTests.cs	`100% <100%> (ø)`	⬆️
...ios/IrisPlantClassificationWithStringLabelTests.cs	`98.63% <100%> (ø)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`100% <100%> (ø)`	⬆️
src/Microsoft.ML.Data/TrainCatalog.cs	`84.18% <100%> (+0.07%)`	⬆️
...ansforms/PermutationFeatureImportanceExtensions.cs	`97.93% <33.33%> (ø)`	⬆️
...crosoft.ML.StandardTrainers/Optimizer/Optimizer.cs	`72.17% <0%> (-1.16%)`	⬇️
... and 2 more

artidoro · 2019-03-20T17:55:38Z

Did you check the RecommenderCatalog? I think it's on a separate file. #Resolved

artidoro

abgoswam · 2019-03-20T18:39:56Z

src/Microsoft.ML.Data/TrainCatalog.cs

@@ -484,21 +484,21 @@ internal MulticlassClassificationTrainers(MulticlassClassificationCatalog catalo
        /// <param name="labelColumnName">The name of the label column in <paramref name="data"/>.</param>
        /// <param name="scoreColumnName">The name of the score column in <paramref name="data"/>.</param>
        /// <param name="predictedLabelColumnName">The name of the predicted label column in <paramref name="data"/>.</param>
-        /// <param name="topK">If given a positive value, the <see cref="MulticlassClassificationMetrics.TopKAccuracy"/> will be filled with
+        /// <param name="topPredictionCount">If given a positive value, the <see cref="MulticlassClassificationMetrics.TopKAccuracy"/> will be filled with


If given a positive value [](start = 45, length = 25)

it seems the existing behavior is to just ignore negative values. is that correct ? should we raise an exception if user gives a negative value #Resolved

No problem. We throw now. #Resolved

rogancarr · 2019-03-20T21:49:54Z

src/Microsoft.ML.Data/TrainCatalog.cs

@@ -484,21 +484,21 @@ internal MulticlassClassificationTrainers(MulticlassClassificationCatalog catalo
        /// <param name="labelColumnName">The name of the label column in <paramref name="data"/>.</param>
        /// <param name="scoreColumnName">The name of the score column in <paramref name="data"/>.</param>
        /// <param name="predictedLabelColumnName">The name of the predicted label column in <paramref name="data"/>.</param>
-        /// <param name="topK">If given a positive value, the <see cref="MulticlassClassificationMetrics.TopKAccuracy"/> will be filled with
+        /// <param name="topPredictionCount">If given a positive value, the <see cref="MulticlassClassificationMetrics.TopKAccuracy"/> will be filled with


Did you also rename TopKAccuracy? If we make this change, we won't have parallelism with the evaluation metrics. #Resolved

We don't need to. TopKAccuracy is as good as Accuracy. This original topK is a parameter when computing TopKAccuracy, not TopKAccuracy itself. #Resolved

Discussed offline. We will do topKPredictionCount to associate it with TopKAccuracy. #Resolved

rogancarr

🎈

Ivanidzo4ka · 2019-03-20T23:59:38Z

test/Microsoft.ML.Tests/AnomalyDetectionTests.cs

@@ -30,7 +30,7 @@ public void RandomizedPcaTrainerBaselineTest()
            var transformedData = DetectAnomalyInMnistOneClass(trainPath, testPath);

            // Evaluate
-            var metrics = ML.AnomalyDetection.Evaluate(transformedData, k: 5);
+            var metrics = ML.AnomalyDetection.Evaluate(transformedData, falsePositiveCount: 5);


falsePositiveCount [](start = 72, length = 18)

AnomalyDetectionMetrics has DetectionRateAtKFalsePositives
If you remove K from here, I think you need to remove K from metric class as well. #Resolved

Let's do DetectionRateAtFalsePositiveCount.

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

Ivanidzo4ka · 2019-03-20T23:59:55Z

src/Microsoft.ML.Data/TrainCatalog.cs

        /// the top-K accuracy, that is, the accuracy assuming we consider an example with the correct class within
        /// the top-K values as being stored "correctly."</param>
        /// <returns>The evaluation results for these calibrated outputs.</returns>
        public MulticlassClassificationMetrics Evaluate(IDataView data, string labelColumnName = DefaultColumnNames.Label, string scoreColumnName = DefaultColumnNames.Score,
-            string predictedLabelColumnName = DefaultColumnNames.PredictedLabel, int topK = 0)
+            string predictedLabelColumnName = DefaultColumnNames.PredictedLabel, int topKPredictionCount = 0)


topKPredictionCount [](start = 85, length = 19)

Can we align it with MulticlassClassificationMetrics class?

/// <summary> /// If positive, this is the top-K for which the <see cref="TopKAccuracy"/> is calculated. /// </summary> public int TopK { get; } ``` #Resolved

No problem.

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

wschin · 2019-03-21T01:02:08Z

Yes, it looks good.

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

wschin requested review from artidoro and abgoswam March 20, 2019 01:16

wschin changed the title ~~Polish train catalog~~ Polish train catalog (renaming only) Mar 20, 2019

Polish train catalog

6ee8a49

wschin force-pushed the polish-train-catalog branch from 1599657 to 6ee8a49 Compare March 20, 2019 15:45

wschin self-assigned this Mar 20, 2019

wschin added the API Issues pertaining the friendly API label Mar 20, 2019

Rename partitionKeyColumnName back to samplingKeyColumnName

5a3372d

artidoro approved these changes Mar 20, 2019

View reviewed changes

abgoswam reviewed Mar 20, 2019

View reviewed changes

rogancarr reviewed Mar 20, 2019

View reviewed changes

Address a comment

0197de6

rogancarr approved these changes Mar 20, 2019

View reviewed changes

Add K back

ea24842

Ivanidzo4ka reviewed Mar 20, 2019

View reviewed changes

More TopKPredictionCount

458dcc6

Merge branch 'master' into polish-train-catalog

2d55c67

wschin merged commit 5b22420 into dotnet:master Mar 21, 2019

wschin deleted the polish-train-catalog branch March 21, 2019 05:41

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish train catalog (renaming only) #3030

Polish train catalog (renaming only) #3030

wschin commented Mar 20, 2019 •

edited

Loading

codecov bot commented Mar 20, 2019 •

edited

Loading

artidoro commented Mar 20, 2019 •

edited by wschin

Loading

artidoro left a comment

abgoswam Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 20, 2019 •

edited

Loading

rogancarr Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 20, 2019 •

edited

Loading

wschin Mar 20, 2019 •

edited

Loading

rogancarr left a comment

Ivanidzo4ka Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 21, 2019

Ivanidzo4ka Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 21, 2019

wschin commented Mar 21, 2019

Polish train catalog (renaming only) #3030

Polish train catalog (renaming only) #3030

Conversation

wschin commented Mar 20, 2019 • edited Loading

codecov bot commented Mar 20, 2019 • edited Loading

Codecov Report

artidoro commented Mar 20, 2019 • edited by wschin Loading

artidoro left a comment

Choose a reason for hiding this comment

abgoswam Mar 20, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

rogancarr Mar 20, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

wschin Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

rogancarr left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Mar 20, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 21, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Mar 20, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

wschin Mar 21, 2019

Choose a reason for hiding this comment

wschin commented Mar 21, 2019

wschin commented Mar 20, 2019 •

edited

Loading

codecov bot commented Mar 20, 2019 •

edited

Loading

artidoro commented Mar 20, 2019 •

edited by wschin

Loading

abgoswam Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 20, 2019 •

edited

Loading

rogancarr Mar 20, 2019 •

edited by wschin

Loading

wschin Mar 20, 2019 •

edited

Loading

wschin Mar 20, 2019 •

edited

Loading

Ivanidzo4ka Mar 20, 2019 •

edited by wschin

Loading

Ivanidzo4ka Mar 20, 2019 •

edited by wschin

Loading