Replacing TryGetColumnIndex with invocations of GetColumnOrNull #2088
Conversation
| MetadataUtils.TryGetCategoricalFeatureIndices(_data.Schema.Schema, featureIndex, out _catsMap); | ||
| var featureCol = _data.Schema.Schema.GetColumnOrNull(DefaultColumnNames.Features); | ||
| ch.Assert(featureCol.HasValue); | ||
| MetadataUtils.TryGetCategoricalFeatureIndices(_data.Schema.Schema, featureCol.Value.Index, out _catsMap); |
There was a problem hiding this comment.
In this particular case, you are asserting that it has a value, which is fine, but if the purpose is to just to clarify this, you could probably just get away with just doing .Value at the end and operating over that, since that also clarifies, "yes, I definitely expect this to have a value." #Resolved
| int predictedLabelCol; | ||
| if (!input.Data.Schema.TryGetColumnIndex(input.PredictedLabelColumn, out predictedLabelCol)) | ||
| var predictedLabelCol = input.Data.Schema.GetColumnOrNull(input.PredictedLabelColumn); | ||
| if (!predictedLabelCol.HasValue) | ||
| throw host.Except($"Column '{input.PredictedLabelColumn}' not found."); |
There was a problem hiding this comment.
Except [](start = 27, length = 6)
I'm not sure if you want to do this at the same time, but in the general movement to start using ExceptSchemaMismatch to ensure greater uniformity between error messages, you might consider doing that transition at the same time. But, not sure if you want to.
| @@ -381,9 +381,12 @@ private static void GetColTypesAndIndex(IHostEnvironment env, IDataView inputDat | |||
|
|
|||
| for (int i = 0; i < columns.Length; i++) | |||
| { | |||
| if (!inputSchema.TryGetColumnIndex(columns[i].Input, out cols[i])) | |||
| var col = inputSchema.GetColumnOrNull(columns[i].Input); | |||
| if (!col.HasValue) | |||
| throw env.ExceptSchemaMismatch(nameof(inputSchema), "input", columns[i].Input); | |||
There was a problem hiding this comment.
ExceptSchemaMismatch [](start = 30, length = 20)
This would be an example of a current existing usage of ExceptSchemaMismatch, perhaps a pattern to follow in the other places where we are currently using Except. #Resolved
There was a problem hiding this comment.
| trainData.Schema.Schema.TryGetColumnIndex(DefaultColumnNames.Features, out int featureIndex); | ||
| MetadataUtils.TryGetCategoricalFeatureIndices(trainData.Schema.Schema, featureIndex, out categoricalFeatures); | ||
| var featureCol = trainData.Schema.Schema.GetColumnOrNull(DefaultColumnNames.Features); | ||
| ch.Assert(featureCol.HasValue); |
There was a problem hiding this comment.
ch.Assert(featureCol.HasValue); [](start = 16, length = 31)
Similar note, can probably just use .Value in the line above to clarify "yes I expect this to have a value." But, as you like. #Resolved
|
Looks more or less good @sfilipi ! One thing though, you seem to be when you definitely know the column is there, still using this This would be simpler, and possibly more idiomatic with much of our usage elsewhere. What do you think? #Resolved |
|
Good point! thanks for suggesting it! In reply to: 452842662 [](ancestors = 452842662) |
ghost
locked as resolved and limited conversation to collaborators
Work towards #1500 by removing references of TryGetColumnIndex, and replacing them with GetColumnOrNull in a few projects.