Exposing the confusion matrix #3250

sfilipi · 2019-04-08T23:28:49Z

Fixes #2009 by exposing the confusion matrix.

sfilipi · 2019-04-08T23:31:38Z

I am sending this as not final, because i wanted to get early feedback on how much to expose, and where to place it. I still have to document it and add tests before checking in.

I am not adding support to build the weighted confusion matrix, or adding capabilities to sample a subset of classes for it thinking that it might be enough for V1. The infrastructure is in there, let me know if it is desired. #Resolved

codecov · 2019-04-09T00:05:41Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #3250   +/-   ##
=========================================
  Coverage          ?   72.71%           
=========================================
  Files             ?      808           
  Lines             ?   145289           
  Branches          ?    16237           
=========================================
  Hits              ?   105640           
  Misses            ?    35232           
  Partials          ?     4417

Flag	Coverage Δ
#Debug	`72.71% <81.43%> (?)`
#production	`68.23% <72.56%> (?)`
#test	`88.99% <100%> (?)`

Impacted Files	Coverage Δ
.../Evaluators/Metrics/BinaryClassificationMetrics.cs	`95.23% <100%> (ø)`
test/Microsoft.ML.Functional.Tests/Common.cs	`98.38% <100%> (ø)`
...luators/Metrics/MulticlassClassificationMetrics.cs	`100% <100%> (ø)`
...sts/Scenarios/Api/Estimators/PredictAndMetadata.cs	`100% <100%> (ø)`
test/Microsoft.ML.Functional.Tests/Evaluation.cs	`100% <100%> (ø)`
...s/Metrics/CalibratedBinaryClassificationMetrics.cs	`100% <100%> (ø)`
...ta/Evaluators/MulticlassClassificationEvaluator.cs	`77.73% <100%> (ø)`
src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs	`73.65% <66%> (ø)`
...soft.ML.Data/Evaluators/Metrics/ConfusionMatrix.cs	`70% <70%> (ø)`
...ft.ML.Data/Evaluators/BinaryClassifierEvaluator.cs	`77.18% <77.77%> (ø)`

TomFinley · 2019-04-09T17:23:57Z

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

+    {
+        public double[] PrecisionSums { get; }
+        public double[] RecallSums { get; }
+        public ReadOnlyMemory<char>[] Labels => PredictedLabelNames.ToArray(); //sefilipi: return as string[]


ReadOnlyMemory [](start = 15, length = 14)

Remember, as before, that we provide access to the columns, but do not copy out the annotations data. Remember the discussion around having names.

Remember also, as we've found in #3101 that the labels may not even be text, or even exist. #Pending

I looked a bit on how is the ConfusionMatrix idv build, and it looks like an indicator for the predictedLabels will always be there, and it will be of the ReadOnlyMemory<char> type.

https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Data/Evaluators/MulticlassClassificationEvaluator.cs#L222
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Data/Evaluators/BinaryClassifierEvaluator.cs#L361

The metrics aggregators have the class names in the ctor arguments, and if the class names are missing, those get generated from the size of the Score column.

https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Data/Evaluators/MulticlassClassificationEvaluator.cs#L114
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Data/Evaluators/BinaryClassifierEvaluator.cs#L184

So given that, i left the labels as ReadOnlyMemory<char> instead of providing the annotations of the Count column, and just renamed it to PredictedClassIndicators, since they are not going to be the class names all the time.

Let me know if I am missing something else.

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

I looked a bit on how is the ConfusionMatrix idv build, and it looks like an indicator for the predictedLabels will always be there, and it will be of the ReadOnlyMemory<char> type.

Yes, but AFAIK that is mostly because it existed in order to provide a string to write to console, for the console application, back when this was purely a command line tool and not an API. So it has to have some string description, that doesn't mean that it's a good string description. I think in fact it'll just render the number as a string in case there is none. So... not something we want to expose as part of the API! #Closed

TomFinley · 2019-04-09T17:24:51Z

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

+
+        public override string ToString() => MetricWriter.GetConfusionTableAsString(this, false);
+
+        public double GetCountForClassPair(string predictedLabel, string actualLabel)


string predictedLabel, string actualLabel [](start = 43, length = 41)

As with all the remainder, we index slots by numeric index. They may optionally have a string label (or not), but that is not a requirement, and what the string label is (again, if it even exists) cannot be used as any sort of primary key. #Closed

TomFinley · 2019-04-09T17:25:15Z

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

+            Binary = binary;
+        }
+
+        public override string ToString() => MetricWriter.GetConfusionTableAsString(this, false);


MetricWriter.GetConfusionTableAsString [](start = 45, length = 38)

This is dangerous. Do we consider ToString overrides if they change, breaking changes? #Closed

You're right, it is a breaking change.
I'll create a separate API for it.

"Changing the returned values for a property, field, return or 'out' value, such as the value returned from ToString"
on
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md

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

TomFinley · 2019-04-09T17:26:10Z

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

+{
+    public sealed class ConfusionMatrix
+    {
+        public double[] PrecisionSums { get; }


double[] [](start = 15, length = 8)

Arrays are mutable -- never ever make them part of the public surface of an API that's intended to be an immutable structure. #Closed

TomFinley · 2019-04-09T17:29:30Z

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

+    {
+        public double[] PrecisionSums { get; }
+        public double[] RecallSums { get; }
+        public ReadOnlyMemory<char>[] Labels => PredictedLabelNames.ToArray(); //sefilipi: return as string[]


ToArray(); [](start = 68, length = 10)

THis should not exist anyway, but note that we generally there is an expectation in C# APIs that property accesses are "lightweight" in terms of their memory footprint. #Closed

TomFinley · 2019-04-09T17:31:04Z

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

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using System.Linq;
 using Microsoft.ML.Runtime;


If I look through this file, it's unclear to me which of the changes require this. #Closed

TomFinley · 2019-04-09T17:33:45Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            var weightColumn = confusionDataView.Schema.GetColumnOrNull(MetricKinds.ColumnNames.Weight);
+            if (getWeighted)
+                host.CheckParam(weightColumn.HasValue, nameof(getWeighted), "There is no Weight column in the confusionMatrix data view.");
+


Sorry, is this something we anticipate might actually happen? We control the creation of this matrix, as far as I am aware, and this is a purely internal utility.

There are two possible answer to your questions, "no" in which case this should have been an assert (with the message as a comment, not a string!), or "yes" in which case this is something the user will potentially have to deal with, in which case having something that user might have a prayer of understanding would be required. (I do not view this message as actionable or even diagnostic in any way from a user's perspective.)

Same for other check below... #Closed

TomFinley · 2019-04-09T18:02:02Z

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

+
+        public double GetCountForClassPair(string predictedLabel, string actualLabel)
+        {
+            int predictedLabelIndex = PredictedLabelNames.IndexOf(predictedLabel.AsMemory());


predictedLabel.AsMemory() [](start = 66, length = 25)

This API is not what we want anyway (since it relies on strings to index when there's no requirement that the strings even exist much less be unique), but in point of interest, please note that this code absolutely will not work, at least, not reliably in and sensible or predictable fashion.

var m = "hello my friend".AsMemory(); var tokens = new List<ReadOnlyMemory<char>>(); tokens.Add(m.Slice(0, 5)); tokens.Add(m.Slice(6, 2)); tokens.Add(m.Slice(9, 6)); foreach (var token in tokens) Console.WriteLine($"'{token}'"); Console.WriteLine(tokens.IndexOf("friend".AsMemory())); Console.WriteLine(tokens.IndexOf(tokens[2]));

If compiled and run on .NET core 2.1, you get this output.

'hello' 'my' 'friend' -1 2

So, relying on ReadOnlyMemory<char>'s equality w.r.t. other similar items just is not a safe thing to do at all. You'll also recall all the bugs and barriers we had to fix back when we did the initial switch from our own DvText to ReadOnlyMemory<char>. #Closed

TomFinley · 2019-04-12T17:22:58Z

src/Microsoft.ML.Core/Data/AnnotationUtils.cs

@@ -440,7 +440,7 @@ public static IEnumerable<SchemaShape.Column> GetTrainerOutputAnnotation(bool is
        /// <param name="labelColumn">Label column.</param>
        public static IEnumerable<SchemaShape.Column> AnnotationsForMulticlassScoreColumn(SchemaShape.Column? labelColumn = null)
        {
-            var cols = new List<SchemaShape.Column>();
+           var cols = new List<SchemaShape.Column>();


As far as I see the only change in this file is this incorrect indentation. Perhaps we want to fix this? #Closed

shauheen · 2019-04-12T17:29:13Z

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

+using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using Microsoft.ML.Runtime;


Please add the header lines #Resolved

TomFinley · 2019-04-12T17:50:14Z

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

+        /// <summary>
+        /// The calculated value of <a href="https://en.wikipedia.org/wiki/Precision_and_recall#Precision">precision</a> for each class.
+        /// </summary>
+        public ImmutableArray<double> PerClassPrecision { get; }


ImmutableArray [](start = 15, length = 14)

That's fine, note that elsewhere in similar situations we've been using IReadOnlyList, but I guess this works too? #Closed

I would also prefer to expose IReadOnlyList just for the sake of consistency. (I don't think we use ImmutableArray in public interface in metrics at all).

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

So @sfilipi I notice this was marked as resolved, if you disagree with me and @Ivanidzo4ka that's fine, but would you mind sharing justification? It is somewhat inconsistent?

In reply to: 275082837 [](ancestors = 275082837,275005362)

Ivan has commented after i marked it as resolved, and i marked it as resolved because i thought your comment was just an observation, not a suggestion for change. I'll swap it.

We do use the ImmutableArrays in the Normalizers.

In reply to: 275472667 [](ancestors = 275472667,275082837,275005362)

Did the change. Just wanted to mention that the reason i used arrays in the first place, is because i believe arrays are preferred over lists when it comes to small collections that don't get updated frequently.

In reply to: 275504885 [](ancestors = 275504885,275472667,275082837,275005362)

Thanks @sfilipi. You may be right, but for things like this I feel like using the more "consistent" solution with what we've done elsewhere, may be more desirable. (Even if one or two places we do use immutable arrays. See also VectorDataViewType.Dimensions, but that one was deliberate.)

It may be that in future we may decide using immutable arrays is more desirable. Incidentally this is allowed by the rules is "Returning a value of a more derived type for a property, field, return or out value", so in principle we could change what we think of as read only lists to immutable arrays.

In reply to: 275554556 [](ancestors = 275554556,275504885,275472667,275082837,275005362)

TomFinley · 2019-04-12T17:51:09Z

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

+        /// <param name="predictedClassIndicatorIndex">The index of the predicted label indicator, in the <see cref="PredictedClassesIndicators"/>.</param>
+        /// <param name="actualClassIndicatorIndex">The index of the actual label indicator, in the <see cref="PredictedClassesIndicators"/>.</param>
+        /// <returns></returns>
+        public double GetCountForClassPair(uint predictedClassIndicatorIndex, uint actualClassIndicatorIndex)


actualClassIndicatorIndex [](start = 83, length = 25)

Indices are usually int, as far as I understand. I understand that as far as they are key types they are represented as int, but when it comes down to, say, doing slot indexing or whatever, they're ints. What you think? #Closed

Also, might be nice if there was a special property to indicate the number of classes, maybe. I guess they can get it from the above lists, but, might be nice otherwise.

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

TomFinley · 2019-04-12T17:53:04Z

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

+
+        private readonly IHost _host;
+
+        internal ConfusionMatrix(IHost host, double[] precision, double[] recall, double[][] confusionTableCounts,


precision [](start = 54, length = 9)

Might be nice to have some asserts here to make poor readers of the code (like me!) aware of the intent here -- I assume precision and recall have the same length, that confusionTableCounts is a jagged array with that same length along both dimensions, but these are just guesses based on my understanding of the setting, which many people coming to the code fresh would not have. And even I'm not sure. #Closed

sfilipi · 2019-04-12T21:20:24Z

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

+        /// It contains information about the predicted classes, if that information is available.
+        /// It can be the classes names, or their indices.
+        /// </summary>
+        public DataViewSchema.Annotations ClassIndicators { get; }


public DataViewSchema.Annotations ClassIndicators { get; } [](start = 7, length = 59)

@[email protected] is exposing this desirable? #Resolved

I'd prefer to not? AFAIK you get the class indicators out of the column they pass to the evaluate method. So why store it here? Why not just say, "get the indicators?" What does this API buy you? So my initial reaction is, "no, why is this object so complicated?"

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

I mean, if I think about a confusion matrix, all I really want is the matrix, maybe a few summary statistics. Making this API for this limited object responsible for querying the metadata on the predicted label column when we already have APIs to do that that are more general seems like it is just making the story harder than it needs to be.

In reply to: 275473328 [](ancestors = 275473328,275067993)

sfilipi · 2019-04-12T21:21:31Z

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

+        /// The indicators of the predicted classes.
+        /// It might be the classes names, or just indices of the predicted classes, if the name mapping is missing.
+        /// </summary>
+        internal IReadOnlyList<ReadOnlyMemory<char>> PredictedClassesIndicators;


PredictedClassesIndicators [](start = 53, length = 26)

if the above is ok, i can remove this, and just pass in the annotations everywhere. #Resolved

Ivanidzo4ka · 2019-04-12T22:17:17Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            // if there is a Weight column, return the weighted confusionMatrix as well, from this function.
+            if (isWeighted)
+            {
+                confusionMatrix = GetConfusionTableAsType(host, confusionDataView, binary, sample, false);


confusionMatrix = GetConfusionTableAsType(host, confusionDataView, binary, sample, false); [](start = 16, length = 90)

why we doing this again?
We already did that in line 1359 #Resolved

it should be true instead of false.

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

Ivanidzo4ka · 2019-04-12T22:19:47Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            return confusionTableString;
+        }
+
+        public static ConfusionMatrix GetConfusionTableAsType(IHost host, IDataView confusionDataView, bool binary = true, int sample = -1, bool getWeighted = false)


GetConfusionTableAsType [](start = 38, length = 23)

Maybe it make sense to rename this method to GetConfusionMatrix and GetConfusionTable to GetConfusionMatrixAsString or GetConfusionString? #Resolved

GetConfusionMatrixAsString exists. So I used GetConfusionTableAsFormattedString.

and I hate it already. lmk if anything better comes to mind.

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

Can you at least rename this one to GetConfusionMatrix ?

In reply to: 275088372 [](ancestors = 275088372,275080299)

Ivanidzo4ka · 2019-04-12T22:24:55Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

@@ -1553,13 +1581,13 @@ private static string GetFoldMetricsAsString(IHostEnvironment env, IDataView dat
        }

        // Get a string representation of a confusion table.
-        private static string GetConfusionTableAsString(double[][] confusionTable, double[] rowSums, double[] columnSums,
-            List<ReadOnlyMemory<char>> predictedLabelNames, string prefix = "", bool sampled = false, bool binary = true)
+        internal static string GetConfusionTableAsString(ConfusionMatrix confusionMatrix, bool isWeighted)


GetConfusionTableAsString [](start = 31, length = 25)

Mix of ConfusionMatrix and ConfusionTable makes me
...
http://i.imgur.com/lgSDkvC.gif
...
Confuse!

there is also confusionDataView

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

Ivanidzo4ka · 2019-04-12T22:27:01Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

@@ -1591,10 +1619,11 @@ private static string GetFoldMetricsAsString(IHostEnvironment env, IDataView dat
            else
                rowLabelFormat = string.Format("{{1,{0}}} ||", paddingLen);

+            var confusionTable = confusionMatrix.ConfusionTableCounts;


var confusionTable = confusionMatrix.ConfusionTableCounts; [](start = 12, length = 58)

SO MUCH CONFUSION!
no, seriously, how confusionTable can be ConfusionTableCounts in ConfusionMatrix? #Resolved

Ivanidzo4ka · 2019-04-12T22:28:22Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

            var sb = new StringBuilder();
-            if (numLabels == 2 && binary)
+            if (numLabels == 2 && confusionMatrix.Binary)


IsBinary? #Resolved

Ivanidzo4ka · 2019-04-12T22:29:44Z

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

@@ -74,6 +74,8 @@ public class BinaryClassificationMetrics
        /// </remarks>
        public double AreaUnderPrecisionRecallCurve { get; }

+        public ConfusionMatrix ConfusionMatrix { get; }


ConfusionMatrix [](start = 31, length = 15)

need documentation #Closed

Ivanidzo4ka · 2019-04-12T22:30:20Z

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

@@ -82,9 +82,12 @@ public sealed class MulticlassClassificationMetrics
        /// </remarks>
        public IReadOnlyList<double> PerClassLogLoss { get; }

-        internal MulticlassClassificationMetrics(IExceptionContext ectx, DataViewRow overallResult, int topKPredictionCount)
+        // The confusion matrix.
+        public ConfusionMatrix ConfusionMatrix { get; }


ConfusionMatrix [](start = 31, length = 15)

need documentation #Resolved

Ivanidzo4ka · 2019-04-12T22:32:42Z

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

            PerClassLogLoss = perClassLogLoss.DenseValues().ToImmutableArray();
+            ConfusionMatrix = MetricWriter.GetConfusionTableAsType(host, confusionMatrix, binary: false, perClassLogLoss.Length, getWeighted:false);


false [](start = 141, length = 5)

it's false by default, you can probably omit it. #Resolved

Ivanidzo4ka · 2019-04-15T17:20:08Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            // Get the counts names.
+            var countColumn = confusionDataView.Schema[MetricKinds.ColumnNames.Count];
+            var type = countColumn.Annotations.Schema.GetColumnOrNull(AnnotationUtils.Kinds.SlotNames)?.Type as VectorDataViewType;
+            host.Assert(type != null && type.IsKnownSize && type.ItemType is TextDataViewType, "The Count column does not have a text vector metadata of kind SlotNames.");


Count [](start = 100, length = 5)

{MetricKinds.ColumnNames.Count} #Resolved

moved it to a comment, per Tom's suggestion.

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

Ivanidzo4ka · 2019-04-15T17:20:36Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            // Get the counts names.
+            var countColumn = confusionDataView.Schema[MetricKinds.ColumnNames.Count];
+            var type = countColumn.Annotations.Schema.GetColumnOrNull(AnnotationUtils.Kinds.SlotNames)?.Type as VectorDataViewType;
+            host.Assert(type != null && type.IsKnownSize && type.ItemType is TextDataViewType, "The Count column does not have a text vector metadata of kind SlotNames.");


SlotNames [](start = 158, length = 9)

AnnotationUtils.Kinds.SlotNames #Resolved

moved it to a comment, per Tom's suggestion.

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

Ivanidzo4ka · 2019-04-15T17:24:16Z

test/Microsoft.ML.Functional.Tests/Common.cs

+                Assert.InRange(recall, 0, 1);
+
+
+            // Get the values in the annotations


DOUBLE LINES! DOUBLE SHOCK! #Resolved

ofc it'll happen, there is not compiler complaining about it :)

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

Ivanidzo4ka · 2019-04-15T17:33:35Z

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

+        private readonly IHost _host;
+
+        /// <summary>
+        /// The confusion matrix as a structured type, built from the counts of the confusion table idv.


idv [](start = 100, length = 3)

We don't have idv here. Or what is idv? #Pending

Ivanidzo4ka · 2019-04-15T17:34:10Z

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

+        /// <param name="labelNames">The predicted classes names, or the indexes of the classes, if the names are missing.</param>
+        /// <param name="isSampled">Whether the classes are sampled.</param>
+        /// <param name="isBinary">Whether the confusion table is the result of a binary classification. </param>
+        /// <param name="classIndicators">The Annotations of the Count column, in the confusionTable idv.</param>


confusionTable idv [](start = 86, length = 18)

<paramref name="confusionTableCounts"/>? #Pending

I meant the Annotations in the original IDV that gets created by the evaluator. Expanding the comment with the above note. See if what i put in now is too much.

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

Actually removed the param, so description went away too.

In reply to: 275527727 [](ancestors = 275527727,275468277)

Ivanidzo4ka · 2019-04-15T17:37:13Z

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

+        /// Returns a human readable representation of the confusion table.
+        /// </summary>
+        /// <returns></returns>
+        public string GetFormattedConfusionTable() => MetricWriter.GetConfusionTableAsString(this, false);


MetricWriter.GetConfusionTableAsString [](start = 54, length = 38)

Can we cache it?
Or just calculate it constructor?
I'm not sure in which case this would produce you different results in different times of call. #Resolved

caching it and calculating it when first asked for.

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

TomFinley · 2019-04-15T17:52:04Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            // Get the counts names.
+            var countColumn = confusionDataView.Schema[MetricKinds.ColumnNames.Count];
+            var type = countColumn.Annotations.Schema.GetColumnOrNull(AnnotationUtils.Kinds.SlotNames)?.Type as VectorDataViewType;
+            host.Assert(type != null && type.IsKnownSize && type.ItemType is TextDataViewType, "The Count column does not have a text vector metadata of kind SlotNames.");


, "The Count column does not have a text vector metadata of kind SlotNames." [](start = 93, length = 76)

Assert mesages are pointless so I'd prefer to not have them, and instead have a comment, but that's OK. #Closed

TomFinley · 2019-04-15T22:58:24Z

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

+        internal ConfusionMatrix(IHost host, double[] precision, double[] recall, double[][] confusionTableCounts,
+            List<ReadOnlyMemory<char>> labelNames, bool isSampled, bool isBinary, DataViewSchema.Annotations classIndicators)
+        {
+            _host = host;


_host = host; [](start = 12, length = 13)

Not a big deal but while you're at it, might help to Contracts.AssertValue(host) before assigning it. #Closed

sfilipi · 2019-04-16T16:59:56Z

@Ivanidzo4ka @TomFinley as you find time. Thanks.

TomFinley

Thanks @sfilipi for putting up with my awesome commentary. :) Looks good.

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

artidoro · 2019-04-19T22:19:52Z

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

+
+        /// <summary>
+        /// The confsion matrix counts for the combinations actual class/predicted class.
+        /// The actual classes are in the rows of the table, and the predicted classes in the columns.


rows of the table, and the predicted classes in the columns [](start = 42, length = 59)

I guess that what is a row and what is a column is subjective. Maybe we could talk about the outer/inner index or first/second index? #Resolved

artidoro · 2019-04-19T22:30:18Z

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

+        /// The indicators of the predicted classes.
+        /// It might be the classes names, or just indices of the predicted classes, if the name mapping is missing.
+        /// </summary>
+        public int NumberOfPredictedClasses { get; }


Is this just meant to give the number of classes?
Maybe we could simplify both the name and the comment.

Suggestion: NumberOfClasses #Resolved

artidoro · 2019-04-19T22:58:24Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

+            var confusionMatrix = GetConfusionMatrix(host, confusionDataView, binary, sample, false);
+            var confusionTableString = GetConfusionTableAsString(confusionMatrix, false);
+
+            // if there is a Weight column, return the weighted confusionMatrix as well, from this function.


nit: capital letter. #Resolved

artidoro

…ts can run in WIndows debug.

sfilipi changed the title ~~Exposing the confusion matrix~~ [WIP] Exposing the confusion matrix Apr 8, 2019

sfilipi requested review from TomFinley, Ivanidzo4ka and glebuk April 8, 2019 23:29

TomFinley reviewed Apr 9, 2019

View reviewed changes

sfilipi changed the title ~~[WIP] Exposing the confusion matrix~~ Exposing the confusion matrix Apr 12, 2019

TomFinley reviewed Apr 12, 2019

View reviewed changes

shauheen reviewed Apr 12, 2019

View reviewed changes

TomFinley reviewed Apr 12, 2019

View reviewed changes

sfilipi commented Apr 12, 2019

View reviewed changes

Ivanidzo4ka reviewed Apr 12, 2019

View reviewed changes

Ivanidzo4ka reviewed Apr 15, 2019

View reviewed changes

TomFinley reviewed Apr 15, 2019

View reviewed changes

TomFinley approved these changes Apr 18, 2019

View reviewed changes

TomFinley reviewed Apr 18, 2019

View reviewed changes

src/Microsoft.ML.Data/Evaluators/Metrics/ConfusionMatrix.cs Outdated Show resolved Hide resolved

TomFinley reviewed Apr 18, 2019

View reviewed changes

src/Microsoft.ML.Data/Evaluators/Metrics/ConfusionMatrix.cs Outdated Show resolved Hide resolved

sfilipi requested a review from eerhardt April 19, 2019 07:08

artidoro reviewed Apr 19, 2019

View reviewed changes

artidoro approved these changes Apr 19, 2019

View reviewed changes

sfilipi added 9 commits April 19, 2019 16:55

Exposing the confusion matrix

ab146fc

PR comments

076c04f

Changing the single learner from LightGBM to FastTree so that the tes…

0aea0fc

…ts can run in WIndows debug.

Addressing Tom's comments

c435990

Ivan's suggestions

7b34ac6

Pr review comments

6efb8d8

Asserting the host before assigning it

889988c

fixing nits

74090d6

Artidoro's comments

415937d

sfilipi force-pushed the confusionMatrix branch from c05a80c to 415937d Compare April 20, 2019 00:08

sfilipi merged commit 610ffcb into dotnet:master Apr 20, 2019

sfilipi deleted the confusionMatrix branch April 20, 2019 05:57

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


		public override string ToString() => MetricWriter.GetConfusionTableAsString(this, false);

		public double GetCountForClassPair(string predictedLabel, string actualLabel)


		private readonly IHost _host;

		internal ConfusionMatrix(IHost host, double[] precision, double[] recall, double[][] confusionTableCounts,

		PerClassLogLoss = perClassLogLoss.DenseValues().ToImmutableArray();
		ConfusionMatrix = MetricWriter.GetConfusionTableAsType(host, confusionMatrix, binary: false, perClassLogLoss.Length, getWeighted:false);

		Assert.InRange(recall, 0, 1);


		// Get the values in the annotations

Exposing the confusion matrix #3250

Exposing the confusion matrix #3250

Conversation

sfilipi commented Apr 8, 2019

sfilipi commented Apr 8, 2019 • edited Loading

codecov bot commented Apr 9, 2019 • edited Loading

Codecov Report

TomFinley Apr 9, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

shauheen Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

TomFinley Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Ivanidzo4ka Apr 15, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Apr 15, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Apr 15, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Apr 15, 2019 • edited by sfilipi Loading

sfilipi commented Apr 8, 2019 •

edited

Loading

codecov bot commented Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited by sfilipi

Loading

sfilipi Apr 12, 2019 •

edited

Loading

TomFinley Apr 12, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 9, 2019 •

edited

Loading

TomFinley Apr 12, 2019 •

edited

Loading

shauheen Apr 12, 2019 •

edited by sfilipi

Loading

TomFinley Apr 12, 2019 •

edited

Loading

sfilipi Apr 15, 2019 •

edited

Loading

TomFinley Apr 12, 2019 •

edited

Loading

TomFinley Apr 12, 2019 •

edited

Loading

sfilipi Apr 12, 2019 •

edited

Loading

sfilipi Apr 12, 2019 •

edited

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 12, 2019 •

edited

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 12, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

sfilipi Apr 15, 2019 •

edited

Loading

Ivanidzo4ka Apr 15, 2019 •

edited by sfilipi

Loading

TomFinley Apr 15, 2019 •

edited

Loading

TomFinley Apr 15, 2019 •

edited

Loading

artidoro Apr 19, 2019 •

edited by sfilipi

Loading

artidoro Apr 19, 2019 •

edited by sfilipi

Loading

artidoro Apr 19, 2019 •

edited by sfilipi

Loading