Remove ColumnType.RawKind usages Round 2. #2176

eerhardt · 2019-01-17T21:54:13Z

Removes the "easy" usages of ColumnType.RawKind.

Part of the work necessary for #1860 and contributes to #1533.

There are still about 38 usages of ColumnType.RawKind left, but they are a little more involved, and needs refactoring. I will fully remove them in the next PR.

Removes the "easy" usages of ColumnType.RawKind. Part of the work necessary for dotnet#1860 and contributes to dotnet#1533.

TomFinley · 2019-01-18T04:32:31Z

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

-                case DataKind.U8:
-                    return MakeScalarHashGetter<ulong, HashU8>(input, srcCol, seed, mask);
-                case DataKind.U16:
+            if (srcType.RawType == typeof(byte))


if (srcType.RawType == typeof(byte)) [](start = 12, length = 36)

It used to not matter as much since it was a case statement, but now that it's a chained if statement we probably should give some thought to what is the most common case. The most common case for hashing is text, so we should probably check that first, rather than letting it fall through as the absolute last case when all else fails.

Also probably this would probably be a good time to to checks against the ColumnType instances themselves -- the original author did things in fashion because they considered it to be more concise, but I'm not sure that reasoning holds any longer. #Pending

The most common case for hashing is text, so we should probably check that first,

Good call. Fixing.

Also probably this would probably be a good time to to checks against the ColumnType instances themselves

In my mind, there are 2 reasons why I like comparing the types here, and not against the ColumnType instances.

It is nice and easy to match up the if (.... == typeof(byte)) with the next line MakeScalarHashGetter<byte,..... You can match the byte and byte and easily know you are using the same type.

It is reflective of the code above for KeyType, where you can't actually check for NumberType.U4, since it is a KeyType.

I don't feel strongly about these reasons, but they do seem to be nice. If you'd like it the other way, I can change it. Just let me know if you feel strongly for it to be changed.

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

Fair enough, I think your reasoning works.

In reply to: 249119279 [](ancestors = 249119279,248925374)

TomFinley · 2019-01-18T06:02:52Z

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

@@ -767,7 +767,7 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
            {
                if (!inputSchema.TryFindColumn(colInfo.Input, out var col))
                    throw Host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.Input);
-                if ((col.ItemType.GetItemType().RawKind == default) || !(col.ItemType is VectorType || col.ItemType is PrimitiveType))
+                if (!col.ItemType.GetItemType().RawType.IsValidDataKindType() || !(col.ItemType is VectorType || col.ItemType is PrimitiveType))


col.ItemType.GetItemType().RawType.IsValidDataKindType() || !(col.ItemType is VectorType || col.ItemType is PrimitiveType)) [](start = 21, length = 123)

Not your code, but this test seems completely wrong. As in, almost nothing about this test looks right, or even sensible. col.ItemType could never be a VectorType in any instance, according to its definition. In fact under the current type scheme, discounting any types a user may create themselves and the image type, I think this will succeed on everything??

Actually wait, the error message below is wrong too... it will just claim the column is missing (you see it is identical to the test below)... Many of the metadata tests seem off... And of course it claims that it always produces a vector of known size, but that's definitely not true. Wow. I was going to suggest you fix this when I thought it was just confined to one test, but it seems like literally half the lines in this are flat out wrong. OK. :) I'll file a separate issue...

Filed as #2185. Don't bother fixing of course. :D :D #WontFix

TomFinley · 2019-01-18T06:06:24Z

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

@@ -352,7 +352,7 @@ internal static void SaveType(ModelSaveContext ctx, ColumnType type)
                ctx.Writer.Write(vectorType?.Size ?? 0);

                ColumnType itemType = vectorType?.ItemType ?? type;
-                var itemKind = itemType.RawKind;
+                itemType.RawType.TryGetDataKind(out DataKind itemKind);
                Contracts.Assert(itemKind == DataKind.R4 || itemKind == DataKind.R8);
                ctx.Writer.Write((byte)itemKind);
            }


What a very, very strange format... if there are only two choices, why not just write a bool? Nonetheless, the die is cast, we must live with the format, such as it is...

Given that, I think if we want to get rid of datakind, I think the best choice would be to write the bytes as constants... so I guess, ctx.Writer.Write((byte)(itemType == NumberType.Double ? 10 : 9)) or something like that. I guess we'd need a similar test during the deserialization. Not sure if you're up for this in this PR... maybe the scope here is more limited. #Resolved

Yes, the scope of my current work is to remove ColumnType.RawKind, and not necessarily to remove DataKind altogether. I have a hard time believing we will be able to get rid of it completely. We may be able to internalize it, but there are a few other places that have it exposed publicly - TextLoad, TypeConverting, PartitionedPathParser.

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

TomFinley · 2019-01-18T06:10:20Z

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

@@ -60,7 +60,7 @@ public SchemaShape GetOutputSchema(SchemaShape inputSchema)
                if (!inputSchema.TryFindColumn(colInfo.Input, out var col))
                    throw _host.ExceptSchemaMismatch(nameof(inputSchema), "input", colInfo.Input);

-                if ((col.ItemType.GetItemType().RawKind == default) || !(col.ItemType is VectorType || col.ItemType is PrimitiveType))
+                if (!col.ItemType.GetItemType().RawType.IsValidDataKindType() || !(col.ItemType is VectorType || col.ItemType is PrimitiveType))


if (!col.ItemType.GetItemType().RawType.IsValidDataKindType() || !(col.ItemType is VectorType || col.ItemType is PrimitiveType)) [](start = 16, length = 128)

Oh boy. More similar problems... I'll make a note of it #2185. #WontFix

TomFinley · 2019-01-18T06:16:24Z

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

        {
            VectorType vectorType = type as VectorType;
            ColumnType itemType = vectorType?.ItemType ?? type;

-            if (itemType.RawKind != default && (vectorType != null || type is PrimitiveType))
+            if (itemType.RawType.IsValidDataKindType() && (vectorType != null || type is PrimitiveType))


&& (vectorType != null || type is PrimitiveType) [](start = 54, length = 49)

I could be wrong, but it seems like many of these IsValidDataKindType are actually trying to capture the logic of what you've already written in your IsStandardScalar extension method on ColumnType. That is, "does it have one of the known datakinds," which is functionally the same as "is it one of the known builtin types." I guess you were trying to capture the original method, but I think the two are redundant. And, I'd favor your extension method you introduced two weeks ago to this new method, since this new method seems kind of gnarly.

(Also, incidentally I do not believe, if this first test succeeds, that it is not possible for the statement after the && to be false.)

many of these IsValidDataKindType are actually trying to capture the logic of what you've already written in your IsStandardScalar extension method on ColumnType

That's a good call. I forgot about that method. I've removed IsValidDataKindType and am using IsStandardScalar instead.

I do not believe, if this first test succeeds, that it is not possible for the statement after the && to be false.

Agreed. Removed the 2nd expression.

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

Actually this broke a test. There is a test that passes in a Vec<Key<U4>> into this method. Thus, RawType.IsValidDataKindType and IsStandardScalar are not equivalent because the first allows for KeyTypes.

In reply to: 249152590 [](ancestors = 249152590,248936996)

Huh interesting. Anyway I like your solution to the problem I see below.

In reply to: 249206559 [](ancestors = 249206559,249152590,248936996)

TomFinley · 2019-01-18T06:18:55Z

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

+                    return MakeScalarHashGetter<ushort, HashKey2>(input, srcCol, seed, mask);
+                else if (srcType.RawType == typeof(uint))
+                    return MakeScalarHashGetter<uint, HashKey4>(input, srcCol, seed, mask);
+


By a similar token, I might want to check for uint first, since that is going to be the key type 99% of the time. :) #Resolved

Fixed.

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

TomFinley · 2019-01-18T06:24:06Z

src/Microsoft.ML.Data/DataLoadSave/Binary/Codecs.cs

-                    }
-                    return base.LoadName;
-                }
-            }


Could you make me feel a little bit better about this? Are we sure this won't break binary format backcompat? It just seems strange that we can just pffft get rid of loadnames like this.

Now granted, this code is absolutely archaic so it's entirely possible, even likely, that some parts have somehow become unnecessary or redundant, but, I'd feel better with an explanation. :) #Resolved

Sure, I'll attempt to make you feel better 😄.

UnsafeTypeCodec<T> derives from SimpleCodec<T>. If you look at SimpleCodec<T>.LoadName:

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Binary/Codecs.cs

Lines 117 to 118 in b1cc8eb

// For these basic types, the class name will do perfectly well.

public virtual string LoadName => typeof(T).Name;

Now, Type.RawKind (which this deleted code was using) comes from:

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Binary/Codecs.cs

Lines 178 to 186 in b1cc8eb

// Gatekeeper to ensure T is a type that is supported by UnsafeTypeCodec.

// Throws an exception if T is neither a TimeSpan nor a NumberType.

private static ColumnType UnsafeColumnType(Type type)

{

return type == typeof(TimeSpan) ? (ColumnType)TimeSpanType.Instance : NumberType.FromType(type);

}

public UnsafeTypeCodec(CodecFactory factory)

: base(factory, UnsafeColumnType(typeof(T)))

And there is even an assert that ensures Type.RawType == typeof(T):

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Binary/Codecs.cs

Lines 120 to 127 in b1cc8eb

public SimpleCodec(CodecFactory factory, ColumnType type)

{

Contracts.AssertValue(factory);

Contracts.AssertValue(type);

Contracts.Assert(type.RawType == typeof(T));

Factory = factory;

Type = type;

}

So now, looking at the deleted code, it is just returning typeof(T).Name, which is redundant with what base.LoadName is doing. I started changing this code to check for these 5 types and return Type.RawType.Name, but then discovered this is what the base class is already doing. #Resolved

cc @yaeldekel in case it breaks something in TLC

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

TomFinley · 2019-01-18T06:27:38Z

src/Microsoft.ML.Core/Data/DataKind.cs

+                || type == typeof(ulong)
+                || type == typeof(float)
+                || type == typeof(double)
+                || type == typeof(ReadOnlyMemory<char>) || type == typeof(string)


|| type == typeof(string) [](start = 55, length = 26)

So, can we check this? The raw type should never be a string. I see that above we also somehow map string to a data kind in TryGetDataKind, but that would be a bug. #Resolved

I've deleted this method.

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

TomFinley · 2019-01-18T06:28:35Z

src/Microsoft.ML.Core/Data/ColumnType.cs

+        [BestFriend]
+        internal static PrimitiveType FromType(Type type)
+        {
+            if (type == typeof(ReadOnlyMemory<char>) || type == typeof(string))


|| type == typeof(string)) [](start = 52, length = 27)

This also seems wrong. #Resolved

I've removed || type == typeof(string) from this line. All the callers are guaranteed to do the string => ROM<char> type conversion before making this call.

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

TomFinley · 2019-01-18T06:29:30Z

src/Microsoft.ML.Core/Data/MetadataUtils.cs

@@ -263,7 +263,7 @@ internal static IEnumerable<int> GetColumnSet(this Schema schema, string metadat
            for (int col = 0; col < schema.Count; col++)
            {
                var columnType = schema[col].Metadata.Schema.GetColumnOrNull(metadataKind)?.Type;
-                if (columnType != null && columnType is KeyType && columnType.RawKind == DataKind.U4)
+                if (columnType != null && columnType is KeyType && columnType.RawType == typeof(uint))


columnType != null && [](start = 20, length = 22)

Not a big deal, but I'd expect that if columnType were null, it would fail the columnType is KeyType anyway, so why bother testing that it is null? Similar with the condition about 24 lines up. #Resolved

Fixed.

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

TomFinley · 2019-01-18T06:32:53Z

src/Microsoft.ML.Data/Evaluators/EvaluatorUtils.cs

@@ -183,7 +183,7 @@ private static bool CheckScoreColumnKind(Schema schema, int col)

            // Get the score column set id from colScore.
            var type = schema[colScore].Metadata.Schema.GetColumnOrNull(MetadataUtils.Kinds.ScoreColumnSetId)?.Type;
-            if (type == null || !(type is KeyType) || type.RawKind != DataKind.U4)
+            if (type == null || !(type is KeyType) || type.RawType != typeof(uint))


type == null || [](start = 16, length = 15)

Similar note, I think we can do without the null check given that the is test will catch the null case anyway, I think? #Resolved

Fixed.

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

TomFinley · 2019-01-18T06:43:09Z

src/Microsoft.ML.Data/DataView/InternalSchemaDefinition.cs

-            // Get the data kind, and the item's column type.
-            if (rawItemType == typeof(string))
-                kind = DataKind.Text;
-            else if (!rawItemType.TryGetDataKind(out kind))


This was actually important -- we do not want string associated with the type TextType, because TextType.Instance.RawType is not string, it is ReadOnlyMemory<char>. We need to handle that type mapping "specially" just as we specially map arrays to VBuffer in other code here, and so on, and so on. It is not the job of ColumnType to be aware of the details of how in this convenience API we perform mappings from user types into IDataView land. So if that is why I see those tests against string that I commented on earlier as being odd, let's not do that. #Resolved

Good call. I've added this back, by doing the string => ROM<char> type conversion here.

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

eerhardt · 2019-01-22T19:26:56Z

Looking for reviews and sign offs on this PR, if anyone is available. #Resolved

sfilipi · 2019-01-22T21:33:01Z

src/Microsoft.ML.Data/Model/Pfa/PfaUtils.cs

-                    // REVIEW: This should really be float. But, for the
+                    return Int;
+                }
+                else if (rawType == typeof(uint)


uint [](start = 43, length = 4)

not your change, but i wonder if uint belongs here, and not with the clause above, where typeof(int) is included.
AFAIK int and uint have the same length. Are we going by numeric ranges? @weschin #Resolved

I assume if you are using uint, you have the possibility of having values that are greater than the signed int.MaxValue. Thus, if we did use Int, it would overflow to a negative value, which I'm sure wouldn't produce the correct results.

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

sfilipi · 2019-01-22T21:34:52Z

src/Microsoft.ML.Data/Model/Pfa/PfaUtils.cs

                }
+
+                return null;
            }

            public static JToken DefaultTokenOrNull(PrimitiveType itemType)


DefaultTokenOrNull [](start = 33, length = 18)

can most of this be substituted with default(itemType) #Resolved

No. We could do that if this method was a generic method - DefaultTokenOrNull<T>. Then you could just return default(T). But since the System.Type we are inspecting here isn't known at compile time, you can't use the C# default keyword.

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

sfilipi

TomFinley

Remove ColumnType.RawKind usages Round 2.

d1b9c0a

Removes the "easy" usages of ColumnType.RawKind. Part of the work necessary for dotnet#1860 and contributes to dotnet#1533.

eerhardt requested review from Ivanidzo4ka, TomFinley, yaeldekel and sfilipi January 17, 2019 21:54

TomFinley reviewed Jan 18, 2019

View reviewed changes

TomFinley mentioned this pull request Jan 18, 2019

Conditions in KeyToVectorMappingEstimator.GetOutputSchema seem wrong #2185

Open

TomFinley reviewed Jan 18, 2019

View reviewed changes

eerhardt added 2 commits January 18, 2019 13:51

Respond to PR feedback

1a4875e

Fix type check caught in tests.

1ab90a5

eerhardt mentioned this pull request Jan 22, 2019

Remove ColumnType.RawKind Round 3 (and final) #2202

Merged

sfilipi reviewed Jan 22, 2019

View reviewed changes

sfilipi approved these changes Jan 22, 2019

View reviewed changes

TomFinley approved these changes Jan 23, 2019

View reviewed changes

eerhardt merged commit 925059d into dotnet:master Jan 23, 2019

eerhardt deleted the ColumnTypeRawKind2 branch January 23, 2019 15:15

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ColumnType.RawKind usages Round 2. #2176

Remove ColumnType.RawKind usages Round 2. #2176

eerhardt commented Jan 17, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 22, 2019

TomFinley Jan 18, 2019 •

edited

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

eerhardt Jan 18, 2019

TomFinley Jan 22, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019 •

edited

Loading

sfilipi Jan 22, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019

eerhardt commented Jan 22, 2019 •

edited

Loading

sfilipi Jan 22, 2019 •

edited by eerhardt

Loading

eerhardt Jan 22, 2019

sfilipi Jan 22, 2019 •

edited by eerhardt

Loading

eerhardt Jan 22, 2019

sfilipi left a comment

TomFinley left a comment

	// For these basic types, the class name will do perfectly well.
	public virtual string LoadName => typeof(T).Name;

	// Gatekeeper to ensure T is a type that is supported by UnsafeTypeCodec.
	// Throws an exception if T is neither a TimeSpan nor a NumberType.
	private static ColumnType UnsafeColumnType(Type type)
	{
	return type == typeof(TimeSpan) ? (ColumnType)TimeSpanType.Instance : NumberType.FromType(type);
	}

	public UnsafeTypeCodec(CodecFactory factory)
	: base(factory, UnsafeColumnType(typeof(T)))

	public SimpleCodec(CodecFactory factory, ColumnType type)
	{
	Contracts.AssertValue(factory);
	Contracts.AssertValue(type);
	Contracts.Assert(type.RawType == typeof(T));
	Factory = factory;
	Type = type;
	}

Remove ColumnType.RawKind usages Round 2. #2176

Remove ColumnType.RawKind usages Round 2. #2176

Conversation

eerhardt commented Jan 17, 2019

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

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 Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

eerhardt Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Jan 22, 2019 • edited Loading

sfilipi Jan 22, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 22, 2019 • edited by eerhardt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt Jan 18, 2019 •

edited

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

TomFinley Jan 18, 2019 •

edited by eerhardt

Loading

eerhardt commented Jan 22, 2019 •

edited

Loading

sfilipi Jan 22, 2019 •

edited by eerhardt

Loading

sfilipi Jan 22, 2019 •

edited by eerhardt

Loading