Skip to content

Conversation

@mstfbl
Copy link
Contributor

@mstfbl mstfbl commented Sep 18, 2019

Attempt on Issue #4169

  • Added the necessary estimator pipeline for a KeyDataViewType string/value binding
  • Changed the Output Column types of the ImageClassificationEstimator and the Mapper for future compatibility with KeyType composite values
  • Added test cases to test features added above

fixes #4169

Attempt on Issue 4169

- Added the necessary estimator pipeline for a KeyDataViewType string/value binding
- Changed the Output Column types of the ImageClassificationEstimator and  the Mapper for future compatibility with KeyType composite values
@dnfclas
Copy link

dnfclas commented Sep 18, 2019

CLA assistant check
All CLA requirements met. #Resolved

EvaluateModel(mlContext, testDataset, loadedModel);

VBuffer<ReadOnlyMemory<char>> keys = default;
loadedModel.GetOutputSchema(schema)["Label"].GetKeyValues(ref keys);
Copy link
Member

@codemzs codemzs Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need to do all this if you convert predicted label column from Key to Value. It will output string type that will contain class names that corresponds to key indices. #Resolved

@codemzs
Copy link
Member

codemzs commented Sep 18, 2019

        public UInt32 PredictedLabel;

this needs to be string. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs:308 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

@codemzs
Copy link
Member

codemzs commented Sep 18, 2019

            $"Predicted Label : {originalLabels[index]}");

prediction.PredictedLabel. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs:148 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@yaeldekel
Copy link

yaeldekel commented Sep 19, 2019

                : SchemaShape.Column.VectorKind.VariableVector, _outputTypes[i].GetItemType(), false);

This is wrong. Given how _outputTypes is initialized in line 1169, the outcome of the condition here is always false: _outputType[0] is a variable length vector, and _outputTypes[1] is a scalar. This would make the result always be of kind SchemaShape.Column.VectorKind.VariableVector, which is not the case. #Resolved


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1203 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

_dnnModel = dnnModel;
_tfInputTypes = new[] { TF_DataType.TF_STRING };
_outputTypes = new[] { new VectorDataViewType(NumberDataViewType.Single), NumberDataViewType.UInt32.GetItemType() };
_outputTypes = new DataViewType[] { new VectorDataViewType(NumberDataViewType.Single), new KeyDataViewType(typeof(uint), 5) };
Copy link

@yaeldekel yaeldekel Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_outputTypes [](start = 12, length = 12)

_outputTypes doesn't need to be an array of DataViewType. All the estimator needs to know is whether the length of the score vector is variable or not (seems to me that it is typically not variable, or at least this information can be inferred from the DnnModel).
If you get rid of this field, you will not have to guess the size of the key (which is also not needed by the estimator). #Resolved

Copy link
Contributor Author

@mstfbl mstfbl Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch, thank you! Quick question, the Image Classification example ResnetV2101TransferLearningTrainTestSplit.cs runs perfectly well when I don't further define _outputTypes, i,e, delete line 69. Does this also mean than the estimator isn't using this field? #Resolved

Copy link
Contributor Author

@mstfbl mstfbl Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've found that the DataViewType field is needed for the pipeline in ResnetV2101TransferLearningTrainTestSplit.cs to fit properly. #Resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate? The only place _outputTypes is used is in GetOutputSchema (and it is actually being used incorrectly there - see my comments there). What is not working properly in the example you mentioned?


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

Copy link
Contributor Author

@mstfbl mstfbl Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was giving a different error when I removed _outputTypes, but as I now see that was not the bottleneck problem I was having while implementing this KeyType solution. #Resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field _outputTypes can be deleted. The only place where it is used is in line 1281 where you have _outputTypes[0].GetItemType(). Instead of that you can write NumberDataViewType.Single, and then the field is not needed.


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

Copy link
Contributor Author

@mstfbl mstfbl Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this, thank you for the catch! #Resolved

@yaeldekel
Copy link

yaeldekel commented Sep 19, 2019

        for (var i = 0; i < _options.OutputColumns.Length; i++)

How does this work?
It seems like there is an assumption here that the output columns array is always of length 2, and always contains the score column and the predicted label column, in that order. If this is true, then this argument needs to be replaced with two string arguments for the score column name and the predicted label column name. #Resolved


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1199 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

@yaeldekel
Copy link

                : SchemaShape.Column.VectorKind.VariableVector, _outputTypes[i].GetItemType(), false);

This needs to output exactly two SchemaShape.Columns: The first with the score column name, of kind VectorKind.Vector, and the other with the predicted label column name, of kind VectorKind.Scalar, and with isKey set to true.


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


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1203 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

@yaeldekel
Copy link

yaeldekel commented Sep 19, 2019

        _labelColumnName = labelColumnName;

This is not used anywhere but at serialization time. Why does this need to be serialized? #Resolved


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:667 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

@yaeldekel
Copy link

yaeldekel commented Sep 19, 2019

        bool? addBatchDimensionInput, int batchSize, string labelColumnName, string finalModelPrefix, Architecture arch,

This is where we have both the label column name and the training data schema, so this is where we need to extract the key values (if they exist). They will need to be serialized with the transformer, because at load time we don't have access to the training data. #Resolved


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:652 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

@yaeldekel
Copy link

yaeldekel commented Sep 19, 2019

        bool? addBatchDimensionInput, int batchSize, string labelColumnName, string finalModelPrefix, Architecture arch,

I think this argument is not used here. #Resolved


Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:652 in 8d07401. [](commit_id = 8d07401, deletion_comment = False)

Copy link

@yaeldekel yaeldekel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

Here, I isolated the issue of the annotations which carry the KeyValues not copying over to the SchemaShapes in the SchemaShape Create method in IEstimator.cs. In this method, the KeyValues are not being recognized as KeyType, and as a result the annotations are not being copied over.
@mstfbl
Copy link
Contributor Author

mstfbl commented Sep 20, 2019

With my latest commit, I have isolated the issue to the annotations of KeyType not copying over to the SchemaShapes that are being created in line 194 of IEstimator.cs and subsequently in line 61 of EstimatorChains.cs, which are part of the Fitting process in line 81 of ResnetV2101TransferLearningTrainTestSplit.cs . If we can incorporate a way of preserving the original KeyType annotations after the process, we can achieve a KeyType implementation when the Prediction Engine is run in the TrySinglePrediction method of ResnetV2101TransferLearningTrainTestSplit.cs. #Resolved

var info = new DataViewSchema.DetachedColumn[_parent._outputs.Length];
info[0] = new DataViewSchema.DetachedColumn(_parent._outputs[0], new VectorDataViewType(NumberDataViewType.Single, _parent._classCount), null);
info[1] = new DataViewSchema.DetachedColumn(_parent._outputs[1], NumberDataViewType.UInt32, null);
info[1] = new DataViewSchema.DetachedColumn(_parent._outputs[1], new KeyDataViewType(typeof(uint), _parent._classCount), ((DataViewSchema.Column)InputSchema.GetColumnOrNull(_parent._labelColumnName)).Annotations);
Copy link

@yaeldekel yaeldekel Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_labelColumnName [](start = 197, length = 16)

We cannot rely on the _labelColumnName column in the transformer, since it may not be present in InputSchema.
The way this should work is by getting the key values at the time we have the training data (which is required to contain the label column), which is in the constructor of the transformer. The key values should be serialized as part of saving the transformer, so that they are available even when the model is loaded from disk.
You should also be able to remove the _labelColumnName field from the transformer, it is not needed. #Resolved

var info = new DataViewSchema.DetachedColumn[_parent._outputs.Length];
info[0] = new DataViewSchema.DetachedColumn(_parent._outputs[0], new VectorDataViewType(NumberDataViewType.Single, _parent._classCount), null);
info[1] = new DataViewSchema.DetachedColumn(_parent._outputs[1], NumberDataViewType.UInt32, null);
info[1] = new DataViewSchema.DetachedColumn(_parent._outputs[1], new KeyDataViewType(typeof(uint), _parent._classCount), ((DataViewSchema.Column)InputSchema.GetColumnOrNull(_parent._labelColumnName)).Annotations);
Copy link

@yaeldekel yaeldekel Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_outputs [](start = 68, length = 8)

Please replace this field with two fields containing the score column name and the predicted label column name, and do the same for the argument in the Options class.
It is implicitly assumed throughout this code that there are exactly two output columns, the first one being the score column and the second one being the predicted label column - for example, this method (GetOutputColumnsCore), GetOutputSchema in the estimator, GetOutputColumnsCore in the mapper. #Resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that there is the same problem with the input column names - on one hand the argument in the Options class is an array, and in other place it is assumed that it contains exactly one element which is the features column.


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

@yaeldekel
Copy link

SchemaShape is a class that holds some partial information about the schema that the estimators can provide. Estimators are not able to give the exact schema, since they do not see any data until Fit is called. For this reason, they are only able to give information such as:

  • The column that is created is a fixed length vector (what is the length? the estimator doesn't know, it can know only when it sees the data).
  • The column that is created is a key type, and it has an annotation of key values that is a fixed length vector of text (what is the length of the key values vector and what are the actual key values? There is no way to know until we see the data).

Each estimator is in charge of creating the SchemaShape of its output. The EstimatorChain cannot know that it needs to copy annotations from a different column (from the label column to the predicted label column), the ImageClassificationEstimator is the component that has to do that, in its implementation of GetOutputSchema. Same with the transformer - the ImageClassificationTransformer needs to store the key values from the label column when it sees the training data, and pass them to the predicted label column.


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

string[] inputColumnNames, string modelLocation,
bool? addBatchDimensionInput, int batchSize, string labelColumnName, string finalModelPrefix, Architecture arch,
string scoreColumnName, string predictedLabelColumnName, float learningRate, DataViewSchema inputSchema, int? classCount = null, bool loadModel = false,
string scoreColumnName, string predictedLabelColumnName, float learningRate, DataViewSchema inputSchema, int? classCount = null, string[] keyValueAnnotations = null, bool loadModel = false,
Copy link

@yaeldekel yaeldekel Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyValueAnnotations [](start = 150, length = 19)

The transformer can be created in one of two ways: Either by calling Fit on the estimator, or by deserializing from file. In the second case, the key values come from reading them from the model as you have written. In the first case, you need to get them from inputSchema by getting the key value annotations of the training label column. #Resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that it is possible that the label column does not contain key values annotations, which means that the PredictedLabel column will not have them either. You have to account for that at serialization/deserialization time (for example, by serialiazing the number of key value strings, and only then serializing the strings them selves).


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

_classCount = labelCount == 1 ? 2 : (int)labelCount;

//Temporary string array for key values, will replace soon
_keyValueAnnotations = new string[] { "sunflowers", "roses", "dandelion", "daisy", "tulips"};
Copy link

@yaeldekel yaeldekel Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_keyValueAnnotations [](start = 16, length = 20)

It seems that the loadModel argument specifies whether we are training the model or loading it from a file, inside the if in line 718 is where you should extract the key values from the input schema. #Resolved

@@ -684,6 +692,9 @@ internal ImageClassificationTransformer(IHostEnvironment env, Session session, s
throw Host.ExceptSchemaMismatch(nameof(inputSchema), "label", (string)labelColumn.Name, "Key", (string)labelType.ToString());

_classCount = labelCount == 1 ? 2 : (int)labelCount;
Copy link

@yaeldekel yaeldekel Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

labelCount == 1 ? 2 : (int)labelCount [](start = 30, length = 37)

Why?
/cc @codemzs
#Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To account for binary classification case where in the train set all you have seen is just one class :-).


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

@mstfbl mstfbl dismissed yaeldekel’s stale review October 2, 2019 21:11

Implemented changes

Assert.Equal((int)labelCountFirst, predictionFirst.Score.Length);
Assert.Equal((int)labelCountLast, predictionLast.Score.Length);
Assert.True(Array.IndexOf(labels, predictionFirst.PredictedLabel) > -1);
Assert.True(Array.IndexOf(labels, predictionLast.PredictedLabel) > -1);
Copy link
Member

@codemzs codemzs Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add an assert that predictionFirst and predictionLast have the same value? and also put an assert on the value? (it should remain the same since your seed is fixed) #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I meant can you please assert on the value for predictionFirst.PredictedLabel and predictionLast.PredictedLabel ... instead of just checking its greater than -1


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

Host.AssertNonEmpty(_keyValueAnnotations);
Host.Assert(_keyValueAnnotations.Length == _classCount);
for (int j = 0; j < _classCount; j++)
ctx.SaveNonEmptyString(_keyValueAnnotations[j]);
Copy link
Member

@codemzs codemzs Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really guaranteed _keyValueAnnotations is not empty? refer to below lines of code that you have written in the constructor.

VBuffer<ReadOnlyMemory> keysVBuffer = default;
if (inputSchema[labelColumnName].HasKeyValues())
{
inputSchema[labelColumnName].GetKeyValues(ref keysVBuffer);
_keyValueAnnotations = keysVBuffer.DenseValues().ToArray();
} #Resolved

Copy link
Contributor Author

@mstfbl mstfbl Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your suggested changes below, it is now guaranteed that _keyValueAnnotations will not be empty. #Resolved

{
inputSchema[labelColumnName].GetKeyValues(ref keysVBuffer);
_keyValueAnnotations = keysVBuffer.DenseValues().ToArray();
}
Copy link
Member

@codemzs codemzs Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it does not have key values then just add numeric values (0 - classCount -1) as string to have something in there #Resolved

else
{
_keyValueAnnotations = Enumerable.Range(0, _classCount).ToArray().Select(x => x.ToString().AsMemory()).ToArray();
}
Copy link
Member

@codemzs codemzs Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enumerable.Range(0, _classCount).Select(x => x.ToString().AsMemory()).ToArray(); #Resolved

for (int j = 0; j < predictionFirst.Score.Length; j++)
{
predictionFirst.Score[j] = predictionLast.Score[j];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, it's a mistake.

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@codemzs
Copy link
Member

codemzs commented Oct 3, 2019

@mstfbl How come the tests are now failing?

@codemzs
Copy link
Member

codemzs commented Oct 3, 2019

It seems your tests are failing because of the below error:

tarting test: Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowSentimentClassificationTest
[xUnit.net 00:09:54.21] Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassification [FAIL]
X Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowImageClassification [1m 14s]
Error Message:
Assert.Equal() Failure
↓ (pos 0)
Expected: dandelion
Actual: sunflowers
↑ (pos 0)

Can you please make the tests deterministic by always passing the same image?

{
var dir = new DirectoryInfo(directories[j]);
labels[j] = dir.Name;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is problematic, you cannot assume the ValueToKey transform will see the directories as labels in this order. Instead get keys from the IDataview like we were doing in the sample before

VBuffer<ReadOnlyMemory> keys = default;
loadedModel.GetOutputSchema(schema)["Label"].GetKeyValues(ref keys);

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your tests are failing/giving inaccurate results. we want to be sure you are setting annotations correctly before we merge this,.

@mstfbl
Copy link
Contributor Author

mstfbl commented Oct 3, 2019

I just manually tested for predicting two known images for their labels using the master, unmerged code. I tested predicting an image of a daisy and an image of a rose. The predictions here are correct, which means the error is in my additions pertaining to my annotations, and not in the trained model. I have attached a screenshot of this below.

ah

@yaeldekel @eerhardt We've determined that the trained model is performing correctly without the current additions of categorical values to PredictedLabel. Something is going on with my annotation additions, and I can't find the bug. Would you guys be able to help in diagnosing where the bug is in my additions with fresh new eyes?

@mstfbl mstfbl requested a review from yaeldekel October 3, 2019 06:04
@codemzs
Copy link
Member

codemzs commented Oct 3, 2019

@mstfbl Your bug is in predictedLabel value in output cache. In KeyValue type Keys start from 1 but in Tensorflow predictedLabels start from 0. In file: ImageClassificationTransform.cs after Line 915 outputTensor[1].ToScalar(ref _predictedLabel); add _predictedLabel += 1;

learningRate: 0.01f,
metricsCallback: (metrics) => Console.WriteLine(metrics),
// reuseTrainSetBottleneckCachedValues: true,
// reuseValidationSetBottleneckCachedValues: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this

arch: ImageClassificationEstimator.Architecture.ResnetV2101,
// Uncomment reuseTrainSetBottleneckCachedValues and
// reuseValidationSetBottleneckCachedValues to reuse trained model
// for faster debugging.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this

Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@codemzs codemzs merged commit 4618fca into dotnet:master Oct 3, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Image Classification DNN Transfer Learning] - Use PredictedLabel as String/Value instead of an UInt32 (Index))

5 participants