IndexOutOfRange Exception in KeyToVector transformer by Ivanidzo4ka · Pull Request #2681 · dotnet/machinelearning

Ivanidzo4ka · 2019-02-21T18:01:06Z

eerhardt · 2019-02-21T18:11:05Z

                                if (key >= (uint)size)
                                    continue;
                                editor.Values[count] = 1;
-                                editor.Indices[count++] = slot * size + (int)key;


test? #Resolved

It also appears that there are tests failing in CI with this change.

Maybe the right change is to use

machinelearning/src/Microsoft.ML.Core/Data/VBufferEditor.cs

Lines 42 to 44 in 01a362b

/// <param name="requireIndicesOnDense">

/// True means to ensure the Indices buffer is available, even if the buffer will be dense.

/// </param>

codecov · 2019-02-21T19:59:19Z

Codecov Report

Merging #2681 into master will decrease coverage by 1.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2681      +/-   ##
==========================================
- Coverage   72.78%   71.69%   -1.09%     
==========================================
  Files         808      810       +2     
  Lines      145588   142487    -3101     
  Branches    16250    16112     -138     
==========================================
- Hits       105964   102163    -3801     
- Misses      35202    35898     +696     
- Partials     4422     4426       +4

Flag	Coverage Δ
#Debug	`71.69% <100%> (-1.09%)`	⬇️
#production	`67.92% <100%> (-0.37%)`	⬇️
#test	`85.91% <100%> (-3.12%)`	⬇️

Impacted Files	Coverage Δ
...icrosoft.ML.Tests/Transformers/CategoricalTests.cs	`100% <100%> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/Hashing.cs	`92.04% <100%> (-0.29%)`	⬇️
src/Microsoft.ML.Data/Transforms/KeyToVector.cs	`82.07% <100%> (+0.25%)`	⬆️
...soft.ML.Tests/Transformers/CategoricalHashTests.cs	`100% <100%> (ø)`	⬆️
src/Microsoft.ML.Transforms/KeyToVectorMapping.cs	`91.63% <100%> (+0.39%)`	⬆️
...icrosoft.ML.Functional.Tests/Datasets/Sentiment.cs	`0% <0%> (-100%)`	⬇️
....ML.Functional.Tests/Datasets/HousingRegression.cs	`14.28% <0%> (-85.72%)`	⬇️
...rosoft.ML.Data/DataLoadSave/CompositeDataLoader.cs	`28.57% <0%> (-63.1%)`	⬇️
...ft.ML.StaticPipe/WordEmbeddingsStaticExtensions.cs	`0% <0%> (-62.5%)`	⬇️
src/Microsoft.ML.Transforms/ConversionsCatalog.cs	`50% <0%> (-33.34%)`	⬇️
... and 667 more

TomFinley · 2019-03-06T20:28:34Z

Hi @Ivanidzo4ka, is this relevant to #2185? It seems to be touching much of the same code I noted in that issue.

I mentioned you there but perhaps you did not see. At least, I do not see that there is a comment. Maybe you can review and see that all the issues I observed are actually fixed by this? I see some would be, but might be nice to be complete.

wschin · 2019-03-06T22:34:05Z

                    metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.KeyValues, SchemaShape.Column.VectorKind.Vector, TextDataViewType.Instance, false));
-                result[colInfo.Name] = new SchemaShape.Column(colInfo.Name, col.ItemType is VectorType ? SchemaShape.Column.VectorKind.Vector : SchemaShape.Column.VectorKind.Scalar, NumberDataViewType.UInt32, true, new SchemaShape(metadata));
+                result[colInfo.Name] = new SchemaShape.Column(colInfo.Name, col.Kind, NumberDataViewType.UInt32, true, new SchemaShape(metadata));
            }


Does it mean input shape must be scalar? If yes, we need to throw when encountering a vector. #Resolved

No. It means we accept vectors, scalars and varvectors

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

wschin · 2019-03-06T22:42:12Z

                throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.inputColumnName);

We don't we just check if the input is a key?

Refers to: src/Microsoft.ML.Transforms/KeyToVectorMapping.cs:461 in 540b139. [](commit_id = 540b139, deletion_comment = False)

wschin · 2019-03-06T22:42:48Z

                    metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.IsNormalized, SchemaShape.Column.VectorKind.Scalar, BooleanDataViewType.Instance, false));
-                result[colInfo.outputColumnName] = new SchemaShape.Column(colInfo.outputColumnName, SchemaShape.Column.VectorKind.Vector, NumberDataViewType.Single, false, new SchemaShape(metadata));
+                result[colInfo.outputColumnName] = new SchemaShape.Column(colInfo.outputColumnName,
+                    col.Kind == SchemaShape.Column.VectorKind.VariableVector ? SchemaShape.Column.VectorKind.VariableVector : SchemaShape.Column.VectorKind.Vector,


col [](start = 20, length = 3)

This is key-to-vector mapping. Can the input column be a vector-valued?

You can have vector of keys, right?

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

In fact a vector of keys is one of the most important scenarios (e.g., bag of words).

TomFinley · 2019-03-26T14:56:42Z

                    metadata.Add(slotMeta);
                if (colInfo.InvertHash != 0)
                    metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.KeyValues, SchemaShape.Column.VectorKind.Vector, TextDataViewType.Instance, false));
-                result[colInfo.Name] = new SchemaShape.Column(colInfo.Name, col.ItemType is VectorType ? SchemaShape.Column.VectorKind.Vector : SchemaShape.Column.VectorKind.Scalar, NumberDataViewType.UInt32, true, new SchemaShape(metadata));


col.ItemType is VectorType ? SchemaShape.Column.VectorKind.Vector : SchemaShape.Column.VectorKind [](start = 76, length = 97)

Geez, the original author sure got confused.

TomFinley · 2019-03-26T14:58:07Z


-                result[colInfo.Name] = new SchemaShape.Column(colInfo.Name, SchemaShape.Column.VectorKind.Vector, NumberDataViewType.Single, false, new SchemaShape(metadata));
+                result[colInfo.Name] = new SchemaShape.Column(colInfo.Name,
+                    col.Kind == SchemaShape.Column.VectorKind.VariableVector && !colInfo.Bag ? SchemaShape.Column.VectorKind.VariableVector : SchemaShape.Column.VectorKind.Vector,


col.Kind == SchemaShape.Column.VectorKind.VariableVector && !colInfo.Bag ? SchemaShape.Column.VectorKind.VariableVector : SchemaShape.Column.VectorKind.Vector [](start = 20, length = 158)

Um... yeah.if you wanted to explode out this condition a bit with a bunch of clarifying comments I wouldn't object to this becoming more than a one-line statement. I found this slightly hard to unpack. (Correct, I think, just hard to unpack.)

TomFinley · 2019-03-26T15:01:20Z

Hi @Ivanidzo4ka, is this relevant to #2185? It seems to be touching much of the same code I noted in that issue.

Hi, @Ivanidzo4ka, wondering if you've had a chance to give this any thought or attention.

eerhardt · 2019-05-03T22:20:21Z

@Ivanidzo4ka - what's the status of this PR? Can it be updated?

Ivanidzo4ka · 2019-05-06T20:05:04Z

@eerhardt I've update it.

eerhardt · 2019-05-06T20:09:37Z

@@ -1,14 +0,0 @@
-#@ TextLoader{


Can the SingleDebug versions of these two baseline files be deleted as well?

eerhardt · 2019-05-06T20:12:45Z

-                    new InputOutputColumnPair("TermA", "A"),
-                    new InputOutputColumnPair("TermB", "B"),
-                    new InputOutputColumnPair("TermC", "C")
+                    new OneHotEncodingEstimator.ColumnOptions("CatA", "A"),


OneHotEncodingEstimator.ColumnOptions is an internal class. Can we instead write the tests using the public API?

codemzs · 2019-05-22T21:37:16Z

closing this PR since #3763 has these changes in it. I had initially created that PR not knowing there was this PR but then @yaeldekel pointed out this PR also takes care of output schema so I took those changes. I need to check-in my PR as it is blocking 1P customer.

No need to use indices for dense case

805d579

Ivanidzo4ka requested review from TomFinley, eerhardt and yaeldekel February 21, 2019 18:01

Ivanidzo4ka changed the title ~~Fix for 2678~~ IndexOutOfRange Exception in KeyToVector transformer Feb 21, 2019

eerhardt reviewed Feb 21, 2019

View reviewed changes

Ivan Matantsev added 4 commits February 21, 2019 13:19

Add tests

fcdf059

more tests

058eaac

Merge branch 'master' into Ivanidze/KeyToVectorIORBug

055abae

merge with master

acec961

Ivan Matantsev added 2 commits March 6, 2019 12:43

Merge branch 'master' into Ivanidze/KeyToVectorIORBug

34c2f1a

update baselines

540b139

wschin reviewed Mar 6, 2019

View reviewed changes

TomFinley reviewed Mar 26, 2019

View reviewed changes

wschin approved these changes Apr 15, 2019

View reviewed changes

Ivan Matantsev added 2 commits May 6, 2019 10:21

Merge with master

f0f4e6b

fix test

3b1fc7a

eerhardt reviewed May 6, 2019

View reviewed changes

yaeldekel mentioned this pull request May 22, 2019

Access indices array for VBuffer in KeyToVector transformer only when resulting vector is sparse. #3763

Merged

codemzs closed this May 22, 2019

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

	/// <param name="requireIndicesOnDense">
	/// True means to ensure the Indices buffer is available, even if the buffer will be dense.
	/// </param>

Uh oh!

Conversation

Ivanidzo4ka commented Feb 21, 2019

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TomFinley commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wschin Mar 6, 2019 • edited by Ivanidzo4ka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin commented Mar 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Mar 26, 2019

Uh oh!

eerhardt commented May 3, 2019

Uh oh!

Ivanidzo4ka commented May 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eerhardt Feb 21, 2019 •

edited by Ivanidzo4ka

Loading

codecov Bot commented Feb 21, 2019 •

edited

Loading

TomFinley commented Mar 6, 2019 •

edited

Loading

wschin Mar 6, 2019 •

edited by Ivanidzo4ka

Loading

TomFinley Mar 26, 2019 •

edited

Loading