-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added categorical value support for PredictedLabel for the Image Classification Transfer Learning example. Addresses Issue #4169 #4228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
...icrosoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs
Outdated
Show resolved
Hide resolved
...icrosoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs
Outdated
Show resolved
Hide resolved
| EvaluateModel(mlContext, testDataset, loadedModel); | ||
|
|
||
| VBuffer<ReadOnlyMemory<char>> keys = default; | ||
| loadedModel.GetOutputSchema(schema)["Label"].GetKeyValues(ref keys); |
There was a problem hiding this comment.
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
...icrosoft.ML.Samples/Dynamic/ImageClassification/ResnetV2101TransferLearningTrainTestSplit.cs
Outdated
Show resolved
Hide resolved
codemzs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
This is wrong. Given how 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) }; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
How does this work? Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1199 in 8d07401. [](commit_id = 8d07401, deletion_comment = False) |
This needs to output exactly two In reply to: 533174369 [](ancestors = 533174369) Refers to: src/Microsoft.ML.Dnn/ImageClassificationTransform.cs:1203 in 8d07401. [](commit_id = 8d07401, deletion_comment = False) |
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
left a comment
There was a problem hiding this 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.
|
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
|
Each estimator is in charge of creating the 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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"}; |
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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(); | ||
| } |
There was a problem hiding this comment.
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(); | ||
| } |
There was a problem hiding this comment.
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]; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
codemzs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
|
@mstfbl How come the tests are now failing? |
|
It seems your tests are failing because of the below error: tarting test: Microsoft.ML.Scenarios.TensorFlowScenariosTests.TensorFlowSentimentClassificationTest Can you please make the tests deterministic by always passing the same image? |
| { | ||
| var dir = new DirectoryInfo(directories[j]); | ||
| labels[j] = dir.Name; | ||
| } |
There was a problem hiding this comment.
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);
codemzs
left a comment
There was a problem hiding this 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,.
|
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. @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 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, |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this
codemzs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
Attempt on Issue #4169
fixes #4169