IndexOutOfRange Exception in KeyToVector transformer #2681

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

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

src/Microsoft.ML.Data/Transforms/KeyToVector.cs

@@ -573,7 +573,6 @@ private ValueGetter<VBuffer<float>> MakeGetterInd(DataViewRow input, int iinfo)
                                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

src/Microsoft.ML.Data/Transforms/Hashing.cs

@@ -1273,7 +1273,7 @@ public SchemaShape GetOutputSchema(SchemaShape inputSchema)
                    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));
+                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

src/Microsoft.ML.Transforms/KeyToVectorMapping.cs

@@ -466,7 +466,9 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
                        metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.SlotNames, SchemaShape.Column.VectorKind.Vector, keyMeta.ItemType, false));
                if (col.Kind == SchemaShape.Column.VectorKind.Scalar)
                    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

src/Microsoft.ML.Data/Transforms/Hashing.cs

@@ -1273,7 +1273,7 @@ public SchemaShape GetOutputSchema(SchemaShape inputSchema)
                    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

src/Microsoft.ML.Data/Transforms/KeyToVector.cs

                        metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.SlotNames, SchemaShape.Column.VectorKind.Vector, keyMeta.ItemType, false));
                if (!colInfo.Bag && (col.Kind == SchemaShape.Column.VectorKind.Scalar || col.Kind == SchemaShape.Column.VectorKind.Vector))
                    metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.CategoricalSlotRanges, SchemaShape.Column.VectorKind.Vector, NumberDataViewType.Int32, false));
                if (!colInfo.Bag || (col.Kind == SchemaShape.Column.VectorKind.Scalar))
                    metadata.Add(new SchemaShape.Column(AnnotationUtils.Kinds.IsNormalized, SchemaShape.Column.VectorKind.Scalar, BooleanDataViewType.Instance, false));

-                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

test/BaselineOutput/SingleRelease/Categorical/featurized.tsv

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


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

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

test/Microsoft.ML.Tests/Transformers/CategoricalTests.cs

-                    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 eerhardt, TomFinley 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IndexOutOfRange Exception in KeyToVector transformer #2681

IndexOutOfRange Exception in KeyToVector transformer #2681

Ivanidzo4ka commented Feb 21, 2019

eerhardt Feb 21, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Feb 21, 2019

codecov bot commented Feb 21, 2019 •

edited

Loading

TomFinley commented Mar 6, 2019 •

edited

Loading

wschin Mar 6, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Mar 6, 2019

wschin commented Mar 6, 2019

wschin Mar 6, 2019

Ivanidzo4ka Mar 6, 2019

TomFinley Mar 22, 2019

TomFinley Mar 26, 2019

TomFinley Mar 26, 2019 •

edited

Loading

TomFinley commented Mar 26, 2019

eerhardt commented May 3, 2019

Ivanidzo4ka commented May 6, 2019

eerhardt May 6, 2019

eerhardt May 6, 2019

codemzs commented May 22, 2019

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

IndexOutOfRange Exception in KeyToVector transformer #2681

IndexOutOfRange Exception in KeyToVector transformer #2681

Conversation

Ivanidzo4ka commented Feb 21, 2019

eerhardt Feb 21, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2019 • edited Loading

Codecov Report

TomFinley commented Mar 6, 2019 • edited Loading

wschin Mar 6, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin commented Mar 6, 2019

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

TomFinley Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Mar 26, 2019

eerhardt commented May 3, 2019

Ivanidzo4ka commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemzs commented May 22, 2019

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