Fix PFI issue in binary classification #4587

yaeldekel · 2019-12-18T15:10:34Z

This change adds support for running PFI on binary classification models that do not contain a calibrator. Fixes #4517 .

codecov · 2019-12-18T16:13:31Z

Codecov Report

Merging #4587 into master will increase coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #4587      +/-   ##
==========================================
+ Coverage   75.62%   75.64%   +0.02%     
==========================================
  Files         938      938              
  Lines      168618   168649      +31     
  Branches    18208    18224      +16     
==========================================
+ Hits       127523   127582      +59     
+ Misses      36066    36041      -25     
+ Partials     5029     5026       -3

Flag	Coverage Δ
#Debug	`75.64% <85.71%> (+0.02%)`	⬆️
#production	`71.26% <64.7%> (+0.02%)`	⬆️
#test	`90.44% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
...s/Metrics/CalibratedBinaryClassificationMetrics.cs	`62.5% <0%> (-37.5%)`	⬇️
...soft.ML.Tests/PermutationFeatureImportanceTests.cs	`100% <100%> (ø)`	⬆️
...ansforms/PermutationFeatureImportanceExtensions.cs	`97.93% <100%> (ø)`	⬆️
...soft.ML.Transforms/PermutationFeatureImportance.cs	`61.17% <100%> (+1.17%)`	⬆️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs	`85.15% <0%> (-1.75%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`86.1% <0%> (-0.16%)`	⬇️
src/Microsoft.ML.AutoML/Sweepers/Parameters.cs	`85.16% <0%> (+0.84%)`	⬆️
...soft.ML.Transforms/Text/WordEmbeddingsExtractor.cs	`87.52% <0%> (+1.13%)`	⬆️
src/Microsoft.ML.Maml/MAML.cs	`26.21% <0%> (+1.45%)`	⬆️
... and 3 more

antoniovs1029 · 2019-12-18T19:37:44Z

src/Microsoft.ML.Transforms/PermutationFeatureImportanceExtensions.cs

@@ -171,6 +199,23 @@ public static class PermutationFeatureImportanceExtensions
                auprc: a.AreaUnderPrecisionRecallCurve - b.AreaUnderPrecisionRecallCurve);
        }

+        private static CalibratedBinaryClassificationMetrics CalibratedBinaryClassifierDelta(
+            CalibratedBinaryClassificationMetrics a, CalibratedBinaryClassificationMetrics b)


If I open this PR in Visual Studio I can see that this new method has 0 references. Is this intended?

Particularly, notice that the BinaryClassifierDelta (which already existed) gets a reference in the PermutationFeatureImportance<TModel> method you modified whether isCalibratedModel is true or not. So I was wondering when/where is the CalibratedBinaryClassifierDelta method supposed to be called? #Resolved

I have removed it (I added it initially before we decided not to add calibrated metrics to PFI).

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

gvashishtha · 2020-01-02T20:45:15Z

So based on the ML.NET documentation on PFI, it seems that if you were to take the more time-intensive route of exposing the LogLoss, LogLossReduction, and Entropy metrics, there would be an additional way for users to compare the importance of features (rather than simply R^2)?

What's a situation where R^2 would not be sufficient for doing PFI well?

antoniovs1029 · 2020-01-02T22:46:22Z

What's a situation where R^2 would not be sufficient for doing PFI well?

@gvashishtha

For the record, R^2 is not included as a metric inside BinaryClassificationMetrics (link to code) and so it doesn't exist either in BinaryClassificationMetricsStatistics (link). In the tutorial you linked, Regression is used, and in that case R^2 is included inside its metrics statistics (link).

So the PR here would only affect Binary Classification. Currently BinaryClassificationMetricsStatistics only offers metrics for AreaUnderRocCurve, Accuracy, PositivePrecision, PositiveRecall, NegativePrecision, NegativeRecall, F1Score, and AreaUnderPrecisionRecallCurve. When performing PFI on a Binary Classification model, the change in the metrics is computed for all those metrics, and for every feature. So a user could evaluate the PFI results with any of those metrics (though, in practice, I wouldn't know which metrics would actually be used to choose the most important feature, I guess it depends on the specific case the user is using PFI and there's no objective way of saying if they're "sufficient" or not...).

So, the question remains if it's worth it to make breaking changes in the API to also include LogLoss, LogLossReduction, and Entropy metrics when using PFI with a Binary Classification Calibrated model, by using CalibratedBinaryClassificationMetrics instead. The real question would be if those metrics would be valuable to users running PFI.... and since no one has opened an issue about them, I would tend to think that it's better to not make the breaking changes in the API unless people start asking about them.

harishsk · 2020-01-03T01:27:09Z

src/Microsoft.ML.Transforms/PermutationFeatureImportanceExtensions.cs

+                predictionTransformer,
+                data,
+                () => new BinaryClassificationMetricsStatistics(),
+                idv => catalog.EvaluateNonCalibrated(idv, labelColumnName),


Small Nit:
The only difference between the if (isCalibratedModel) and the else case is the idv parameter. Is it possible to make this a bit more readable by factoring out just that line and using a single call to the PermutationFeatureImportance constructor? #Resolved

Resolving this comment, since isCalibratedModel has been removed.

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

harishsk · 2020-01-03T01:37:10Z

test/Microsoft.ML.Tests/PermutationFeatureImportanceTests.cs

@@ -305,6 +305,18 @@ public void TestPfiBinaryClassificationOnSparseFeatures(bool saveModel)

            Done();
        }
+
+        [Fact]
+        public void TestBinaryClassificationWithoutCalibrator()


The test does not Assert anything. Can you please include Asserts for the relevant results that this test is supposed to verify? #Resolved

harishsk · 2020-01-03T01:45:03Z

Breaking the API has a high bar and I think this does not meet that bar. I would suggest leaving it as is or adding a new API that returns the calibrated metrics.

yaeldekel · 2020-01-06T12:22:19Z

I will leave it as is for now, and make a change that ensures the existing API doesn't break only in case we get customer asks.

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

antoniovs1029 · 2020-01-06T22:27:59Z

src/Microsoft.ML.Data/Evaluators/Metrics/CalibratedBinaryClassificationMetrics.cs

+
+        [BestFriend]
+        internal CalibratedBinaryClassificationMetrics(double auc, double accuracy, double positivePrecision, double positiveRecall,
+            double negativePrecision, double negativeRecall, double f1Score, double auprc, double logLoss, double logLossReduction, double entropy)


So I believe this constructor is no longer used (after you removed the other code handling the calibrated case). Is there a reason the keep this constructor?

antoniovs1029

In general it LGTM, only I have the question about if we should keep the CalibratedBinaryClassificationMetrics constructor that is added in this PR.

yaeldekel requested a review from a team as a code owner December 18, 2019 15:10

antoniovs1029 reviewed Dec 18, 2019

View reviewed changes

yaeldekel force-pushed the pfi branch from 60b652e to b68b862 Compare January 2, 2020 13:01

harishsk reviewed Jan 3, 2020

View reviewed changes

yaeldMS added 8 commits January 5, 2020 14:15

Add non-calibrated evaluation to PFI

4ed3ca8

change to always call EvaluateNonCalibrated

fe48872

Add non-calibrated evaluation to PFI

b6ed4b3

change to always call EvaluateNonCalibrated

0559fb7

Add non-calibrated evaluation to PFI

6558551

change to always call EvaluateNonCalibrated

2d53161

Add non-calibrated evaluation to PFI

bd2af8e

Add asserts to unit test

6e53e5b

yaeldekel force-pushed the pfi branch from b395000 to 6e53e5b Compare January 5, 2020 12:15

Remove using statements

8bde46b

antoniovs1029 reviewed Jan 6, 2020

View reviewed changes

antoniovs1029 approved these changes Jan 7, 2020

View reviewed changes

Remove unused ctor

cd7f27c

antoniovs1029 merged commit e38647c into dotnet:master Jan 8, 2020

antoniovs1029 mentioned this pull request Jun 4, 2020

Using PFI with AutoML, possible? #3972

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PFI issue in binary classification #4587

Fix PFI issue in binary classification #4587

yaeldekel commented Dec 18, 2019

codecov bot commented Dec 18, 2019 •

edited

Loading

antoniovs1029 Dec 18, 2019 •

edited by yaeldekel

Loading

yaeldekel Jan 6, 2020

gvashishtha commented Jan 2, 2020

antoniovs1029 commented Jan 2, 2020 •

edited

Loading

harishsk Jan 3, 2020 •

edited by yaeldekel

Loading

yaeldekel Jan 6, 2020

harishsk Jan 3, 2020 •

edited by yaeldekel

Loading

harishsk commented Jan 3, 2020

yaeldekel commented Jan 6, 2020

antoniovs1029 Jan 6, 2020

antoniovs1029 left a comment

Fix PFI issue in binary classification #4587

Fix PFI issue in binary classification #4587

Conversation

yaeldekel commented Dec 18, 2019

codecov bot commented Dec 18, 2019 • edited Loading

Codecov Report

antoniovs1029 Dec 18, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

yaeldekel Jan 6, 2020

Choose a reason for hiding this comment

gvashishtha commented Jan 2, 2020

antoniovs1029 commented Jan 2, 2020 • edited Loading

harishsk Jan 3, 2020 • edited by yaeldekel Loading

Choose a reason for hiding this comment

yaeldekel Jan 6, 2020

Choose a reason for hiding this comment

harishsk Jan 3, 2020 • edited by yaeldekel Loading

Choose a reason for hiding this comment

harishsk commented Jan 3, 2020

yaeldekel commented Jan 6, 2020

antoniovs1029 Jan 6, 2020

Choose a reason for hiding this comment

antoniovs1029 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 18, 2019 •

edited

Loading

antoniovs1029 Dec 18, 2019 •

edited by yaeldekel

Loading

antoniovs1029 commented Jan 2, 2020 •

edited

Loading

harishsk Jan 3, 2020 •

edited by yaeldekel

Loading

harishsk Jan 3, 2020 •

edited by yaeldekel

Loading