Restore OVA ability to preserve key names on predicted label #3101

Ivanidzo4ka · 2019-03-26T22:26:29Z

Fixes #3090.
I found usage of slot names slightly confusing. In same time we have TrainingLabelValues which should do the trick.

codecov · 2019-03-27T01:11:27Z

Codecov Report

Merging #3101 into master will increase coverage by 0.02%.
The diff coverage is 86.15%.

@@            Coverage Diff             @@
##           master    #3101      +/-   ##
==========================================
+ Coverage   72.52%   72.54%   +0.02%     
==========================================
  Files         808      808              
  Lines      144665   144775     +110     
  Branches    16198    16209      +11     
==========================================
+ Hits       104913   105025     +112     
+ Misses      35342    35336       -6     
- Partials     4410     4414       +4

Flag	Coverage Δ
#Debug	`72.54% <86.15%> (+0.02%)`	⬆️
#production	`68.13% <76.92%> (ø)`	⬆️
#test	`88.83% <100%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
test/Microsoft.ML.Functional.Tests/Training.cs	`100% <100%> (ø)`	⬆️
...sts/Scenarios/Api/Estimators/PredictAndMetadata.cs	`100% <100%> (ø)`	⬆️
...classClassification/MulticlassNaiveBayesTrainer.cs	`87.17% <100%> (+0.05%)`	⬆️
...rosoft.ML.Data/Scorers/PredictedLabelScorerBase.cs	`81.71% <100%> (-0.62%)`	⬇️
...ML.Tests/TrainerEstimators/MetalinearEstimators.cs	`100% <100%> (ø)`	⬆️
....ML.Data/Scorers/MulticlassClassificationScorer.cs	`60.13% <63.63%> (+0.48%)`	⬆️
src/Microsoft.ML.Core/Data/AnnotationUtils.cs	`82% <90%> (+0.22%)`	⬆️
.../Microsoft.ML.Tests/TrainerEstimators/SdcaTests.cs	`97.26% <0%> (-2.74%)`	⬇️
...rc/Microsoft.ML.StaticPipe/SdcaStaticExtensions.cs	`81.72% <0%> (-0.61%)`	⬇️
... and 12 more

TomFinley · 2019-03-27T17:08:47Z

src/Microsoft.ML.Data/Scorers/MulticlassClassificationScorer.cs

@@ -450,7 +450,7 @@ internal static ISchemaBoundMapper WrapCore<T>(IHostEnvironment env, ISchemaBoun
                    trainSchema.Label.Value.GetKeyValues(ref value);
                };

-            return LabelNameBindableMapper.CreateBound<T>(env, (ISchemaBoundRowMapper)mapper, type as VectorDataViewType, getter, AnnotationUtils.Kinds.SlotNames, CanWrap);
+            return LabelNameBindableMapper.CreateBound<T>(env, (ISchemaBoundRowMapper)mapper, type as VectorDataViewType, getter, AnnotationUtils.Kinds.TrainingLabelValues, CanWrap);


AnnotationUtils.Kinds.TrainingLabelValues [](start = 130, length = 41)

Hi @Ivanidzo4ka, could you explain this to me a bit more? It looks like we are no longer publishing these as slot names. Is that correct, or do I misread?

More broadly: I understand from the issue that there has been some regression from 0.11, but I am not certain I understand the nature of the regression completely (how did it regress, by which change?). That lack of knowledge makes this code difficult to review for me, even though the change itself is not too large. #Resolved

I ask you some time ago regarding nature of SlotNames, and what they are always should be Text instances. So I made change in CanWrap method which validates what KeyValues are actually TextInstances.
Which bring this bug.
I don't want to continue to use SlotNames, since they are limited by type. In same time TrainingLabelValues don't look like they have any restriction on data type of it content, so I just want to use it. #Resolved

TomFinley · 2019-03-27T17:12:42Z

test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs

+            // In order to do what we need to get TrainingLabelValues from Score column.
+            // TrainingLabelValues on top of Score column represent original labels for i-th value in Score array.
+            VBuffer<ReadOnlyMemory<char>> originalLabels = default;
+            engine.OutputSchema[nameof(IrisPrediction.Score)].Annotations.GetValue(AnnotationUtils.Kinds.TrainingLabelValues, ref originalLabels);


TrainingLabelValues [](start = 105, length = 19)

Is the distinction with slot names that slots names must be text, while these might be any type? That might excuse not using them. But in such a case I'd argue that we should still have the slot names for descriptive user-facing purposes. so I'd like to confirm we're still doing that. #Resolved

No we don't, but I can change that. Any reason while we want continue to propagate slotnames? #Resolved

Well, let's imagine I write out a text file, and I have this scores column. With slot names, I get a descriptive header. Without it I don't. Does that make sense? #Resolved

Only if original labels were string, but ok.
Just before I make crucial mistake.
Do you prefer to have two wrappers on top of multiclass scorer one for TrainingLabelValues one for SlotNames or you would prefer to extend LabelNameBindableMapper to support multiple getters/ metakinds? #Resolved

Multiple metadata kinds is good, thanks Ivan. I believe this is what you did, that is, from the code I read you are propagating the labels always, and propogating slot names if tehy're text, and that seems fine to me.

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

TomFinley · 2019-03-27T17:14:25Z

test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs

-            Assert.True(slotNames.GetItemOrDefault(0).ToString() == "Iris-setosa");
-            Assert.True(slotNames.GetItemOrDefault(1).ToString() == "Iris-versicolor");
-            Assert.True(slotNames.GetItemOrDefault(2).ToString() == "Iris-virginica");
+            Assert.True(originalLabels.GetItemOrDefault(0).ToString() == "Iris-setosa");


Assert.True [](start = 12, length = 11)

XUnit has something called Assert.Equal. The benefit to using something like that is, if the test fails, you get a more descriptive message than merely knowing that a boolean test failed somewhere.

Something to think about when writing tests. #Resolved

TomFinley · 2019-03-27T20:05:55Z

test/Microsoft.ML.Tests/TrainerEstimators/MetalinearEstimators.cs

+        /// Test what OVA preserves key values for label.
+        /// </summary>
+        [Fact]
+        public void OvaKeyNames()


OvaKeyNames [](start = 20, length = 11)

Just curious, I heard something fairly troubling from @eerhardt, could we test this sort of scenario still works for things other than OVA? #Resolved

Any tests which use TestEstimatorCore will compare metadata from expected and resulted output schema.
I've change AnnotationsForMulticlassScoreColumn to always have TrainingLabelValue, and slotNames if key was text type, so technically we check that everywhere where we use TestEstimatorCore

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

eerhardt · 2019-03-27T20:06:23Z

@Ivanidzo4ka - this issue doesn't appear to be OVA specific. I'm also hitting it in upgrading XamlBrewer.Uwp.MachineLearningSample to the latest build. #Resolved

eerhardt · 2019-03-27T20:07:35Z

test/Microsoft.ML.Tests/TrainerEstimators/MetalinearEstimators.cs

+        /// Test what OVA preserves key values for label.
+        /// </summary>
+        [Fact]
+        public void OvaKeyNames()


Can we add this to FunctionalTests instead? Let's not keep adding tests that the product allows InternalsVisibleTo. That way we can test like a customer uses the product. #Resolved

eerhardt · 2019-03-28T17:47:04Z

test/Microsoft.ML.Tests/TrainerEstimators/MetalinearEstimators.cs

+                .Append(ova)
+                .Append(ML.Transforms.Conversion.MapKeyToValue("PredictedLabel"));
+
+            var model = pipeline.Fit(data);


We should do something with the model to ensure it was created correctly. #Resolved

Ivanidzo4ka · 2019-03-28T22:46:21Z

I've check LR, SDCA and multiclass, all of them working fine with my changes.

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

Ivanidzo4ka · 2019-04-01T20:29:09Z

@eerhardt @TomFinley can you take a look on this PR, would be nice to cherry pick it into 0.12

eerhardt · 2019-04-01T20:30:46Z

Can you address this comment? #3101 (comment)

eerhardt 5 days ago
Can we add this to FunctionalTests instead? Let's not keep adding tests that the product allows InternalsVisibleTo. That way we can test like a customer uses the product.
```` #Resolved

TomFinley · 2019-04-01T20:53:55Z

test/Microsoft.ML.Tests/Scenarios/Api/Estimators/PredictAndMetadata.cs

-            engine.OutputSchema[nameof(IrisPrediction.Score)].GetSlotNames(ref slotNames);
+            // In order to do what we need to get TrainingLabelValues from Score column.
+            // TrainingLabelValues on top of Score column represent original labels for i-th value in Score array.
+            VBuffer<ReadOnlyMemory<char>> originalLabels = default;


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

In this particular case we should be propagating both slot names and label names, right? Since they're string in both cases? While I see the point in augmenting the test to cover this new metadata type, is there any particular reason to remove the test that the vector has teh appropriate slot names? #WontFix

This is old scenario test. Purpose of it to show user how to do work with metadata.
In this particular test label is string, and it has slotnames, but it wont in case of non string label.
If I add slotnames here it would be confusing? What for do I get slotnames?

All tests with TestEstimatorCore routing would test on presence of slotnames and TrainingLabelValues. And we have plenty of them,. why should we do anything here with slotname?

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

OK, that's fine. I had the idea that these "showing the user" things were more the point of functional tests, but as you like.

In reply to: 271050961 [](ancestors = 271050961,271046772)

TomFinley

eerhardt · 2019-04-01T22:08:39Z

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

@@ -467,5 +467,37 @@ public void MetacomponentsFunctionAsExpectedOva()
            // Evaluate the model.
            var binaryClassificationMetrics = mlContext.MulticlassClassification.Evaluate(binaryClassificationPredictions);
        }
+
+        /// <summary>
+        /// Training: Meta-compononts function as expected. For OVA (one-versus-all), a user will be able to specify only


(nit) compononts

eerhardt · 2019-04-01T22:09:12Z

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

+        /// binary classifier trainers. If they specify a different model class there should be a compile error.
+        /// </summary>
+        [Fact]
+        public void MetacomponentsFunctionWithKeyHandeling()


(nit) Handeling

how it's different from what I have now?

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

did you mean Handling?

In reply to: 271072076 [](ancestors = 271072076,271069790)

Handeling isn't a word (at least in English).

eerhardt · 2019-04-01T22:10:15Z

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

+            var binaryClassificationPredictions = binaryClassificationModel.Transform(data);
+
+            // Evaluate the model.
+            var binaryClassificationMetrics = mlContext.MulticlassClassification.Evaluate(binaryClassificationPredictions);


Can we spot check some of these values to make sure the code isn't returning garbage?

eerhardt · 2019-04-01T22:11:35Z

...crosoft.ML.StandardTrainers/Standard/MulticlassClassification/MulticlassNaiveBayesTrainer.cs

@@ -131,7 +131,8 @@ private protected override NaiveBayesMulticlassModelParameters TrainModelCore(Tr
                    int size = cursor.Label + 1;
                    Utils.EnsureSize(ref labelHistogram, size);
                    Utils.EnsureSize(ref featureHistogram, size);
-                    Utils.EnsureSize(ref featureHistogram[cursor.Label], featureCount);
+                    if (featureHistogram[cursor.Label] == null)
+                        featureHistogram[cursor.Label] = new int[featureCount];


Was this change discovered by a test?

I wanted to make sure all multiclass learners works fine with my changes (so I run bunch of different one, on my test)
My test has only 2 features, and I got exception.
Mainly because Utils.EnsureSize use 4 as length for array even if you specify 1 or 2 or 3.
It make sense for VBuffer (since we have Count or Length, i'm always confused about which one is actual size of whole collection and which is size of elements in it), not sure why we do it for arrays as well.

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

Would it make sense to put all those tests into regression? That way we can catch bugs like this in the future?

eerhardt · 2019-04-01T22:12:34Z

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

-            if (labelColumn != null && labelColumn.Value.IsKey && NeedsSlotNames(labelColumn.Value))
-                cols.Add(new SchemaShape.Column(Kinds.SlotNames, SchemaShape.Column.VectorKind.Vector, TextDataViewType.Instance, false));
+            if (labelColumn != null && labelColumn.Value.IsKey)
+


It would probably be good to wrap this in curly braces, now that it is more than one line (and that you have this blank line in between the if and the body.

…3101)

Messy mess

33a578e

daholste approved these changes Mar 26, 2019

View reviewed changes

Let's use training labels instead of slot names

a73b04e

Ivanidzo4ka requested review from TomFinley and yaeldekel March 27, 2019 00:04

use TrainingLabelValues in predict and metadata

fbbcfcf

TomFinley reviewed Mar 27, 2019

View reviewed changes

eerhardt reviewed Mar 27, 2019

View reviewed changes

Return slotnames and traininglabels if possible

d46ae57

eerhardt reviewed Mar 28, 2019

View reviewed changes

Ivan Matantsev added 2 commits March 28, 2019 11:02

Let's validate model

2bb3ecc

also fix naive bayes for less than 4 features.

a61b914

use metakind

b68c4e5

TomFinley reviewed Apr 1, 2019

View reviewed changes

shuffle test around

06cce1a

TomFinley approved these changes Apr 1, 2019

View reviewed changes

eerhardt reviewed Apr 1, 2019

View reviewed changes

eerhardt approved these changes Apr 1, 2019

View reviewed changes

address comments

10541d6

Ivanidzo4ka merged commit d2bf3e7 into dotnet:master Apr 1, 2019

shauheen pushed a commit to shauheen/machinelearning that referenced this pull request Apr 2, 2019

Restore OVA ability to preserve key names on predicted label (dotnet#…

8e3d78c

…3101)

TomFinley mentioned this pull request Apr 9, 2019

Exposing the confusion matrix #3250

Merged

TomFinley mentioned this pull request Apr 18, 2019

Relationship between SchemaShape from IEstimator and DataViewSchema from its ITransformer, and resulting fallout #3380

Closed

antoniovs1029 mentioned this pull request Jan 15, 2020

Using an ImageClassificationTrainer with a dataset that only contains one class of images #4660

Closed

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

Restore OVA ability to preserve key names on predicted label #3101

Restore OVA ability to preserve key names on predicted label #3101

Ivanidzo4ka commented Mar 26, 2019 •

edited

Loading

codecov bot commented Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Apr 1, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 28, 2019

eerhardt commented Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 28, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka commented Mar 28, 2019

Ivanidzo4ka commented Apr 1, 2019

eerhardt commented Apr 1, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Apr 1, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Apr 1, 2019

TomFinley Apr 1, 2019 •

edited

Loading

TomFinley left a comment

eerhardt Apr 1, 2019

eerhardt Apr 1, 2019

Ivanidzo4ka Apr 1, 2019

Ivanidzo4ka Apr 1, 2019

eerhardt Apr 1, 2019

eerhardt Apr 1, 2019

eerhardt Apr 1, 2019

Ivanidzo4ka Apr 1, 2019

eerhardt Apr 1, 2019

eerhardt Apr 1, 2019

Restore OVA ability to preserve key names on predicted label #3101

Restore OVA ability to preserve key names on predicted label #3101

Conversation

Ivanidzo4ka commented Mar 26, 2019 • edited Loading

codecov bot commented Mar 27, 2019 • edited Loading

Codecov Report

TomFinley Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Mar 27, 2019 • edited by Ivanidzo4ka Loading

eerhardt Mar 27, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

eerhardt Mar 28, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Mar 28, 2019

Ivanidzo4ka commented Apr 1, 2019

eerhardt commented Apr 1, 2019 • edited by Ivanidzo4ka Loading

TomFinley Apr 1, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Mar 26, 2019 •

edited

Loading

codecov bot commented Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 27, 2019 •

edited

Loading

TomFinley Apr 1, 2019 •

edited

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt commented Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 27, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 28, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt commented Apr 1, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Apr 1, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Apr 1, 2019 •

edited

Loading