Internalize DataKind #2661

wschin · 2019-02-20T17:47:24Z

Fix #2583. In general, we don't want to see DataKind in any public area. Depending on the actual amount of works required, this PR might first address cases mentioned #2583 and then more PRs will be published later.

TextLoader
TypeConverting
Rest (DataKind is internal-best-friend now).

Note that to specify target types in type converters and text loader, we introduce a cleaner version of existing DataKind,

    public enum DataKind : byte 
    { 
        SByte = 1, 
        Byte = 2, 
        Int16 = 3, 
        UInt16 = 4, 
        Int32 = 5, 
        UInt32 = 6, 
        Int64 = 7, 
        UInt64 = 8, 
        Single = 9, 
        Double = 10, 
        String = 11, 
        Boolean = 12, 
        TimeSpan = 13, 
        DateTime = 14, 
        DateTimeOffset = 15, 
    }

and the existing DataKind is renamed as InternalDataKind.
Other user-facing changes were made to work with DataKind. Command line and entry point are not affected at all.

TomFinley · 2019-02-20T20:13:22Z

docs/samples/Microsoft.ML.Samples/Dynamic/Calibrator.cs

@@ -34,8 +34,8 @@ public static void Example()
                HasHeader = true,
                Columns = new[]
                    {
-                        new TextLoader.Column("Sentiment", DataKind.BL, 0),
-                        new TextLoader.Column("SentimentText", DataKind.Text, 1)
+                        new TextLoader.Column("Sentiment", typeof(bool), 0),


In issue #2006, I had a comment. You will note that you are picking the option that I argued was the worst of all possible worlds. It offers absolutely no hints to the user of what might be an acceptable type. How does a user know that a bool is acceptable, but, say, a List<bool> is not>? This is much, much worse than what it is supposedly meant to replace. #Resolved

Ok. I will make ColumnInfo strongly-typed based on the desired output type. Users will have to create, for example, ToFloatColumnInfo, if they want to conver things to float.

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

That is one possibility. We should also remain open to the possibility that DataKind will persist, just in a changed form. So while doing this work, think about whether it is making the usage more or less clear.

It is a perfectly acceptable outcome to say, "you know what, this enumeration is still useful, just in a vastly abbreviated scope," if we can make that argument. It is unclear to me which is correct. I would not start with the assumption that datakind needs to be internalized and destroyed. (W.r.t. its prior role in ColumnType, absolutely!!) #Resolved

Maybe we can replace Type with PrimitiveDataViewType?

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

Discussed offline. We decided internalizing DataKind and creating an simplified alternative.

In reply to: 259025888 [](ancestors = 259025888,258755422)

codecov · 2019-02-22T00:26:00Z

Codecov Report

Merging #2661 into master will decrease coverage by 0.01%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##           master    #2661      +/-   ##
==========================================
- Coverage    71.7%   71.68%   -0.02%     
==========================================
  Files         808      808              
  Lines      142221   142214       -7     
  Branches    16131    16131              
==========================================
- Hits       101981   101953      -28     
- Misses      35799    35824      +25     
+ Partials     4441     4437       -4

Flag	Coverage Δ
#Debug	`71.68% <73.52%> (-0.02%)`	⬇️
#production	`67.94% <54.82%> (-0.03%)`	⬇️
#test	`85.86% <87.69%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
src/Microsoft.ML.StaticPipe/StaticSchemaShape.cs	`76.5% <ø> (ø)`	⬆️
test/Microsoft.ML.Functional.Tests/Common.cs	`98.54% <ø> (-0.02%)`	⬇️
...ML.Data/Transforms/ConversionsExtensionsCatalog.cs	`76.47% <ø> (ø)`	⬆️
src/Microsoft.ML.Recommender/RecommenderUtils.cs	`57.14% <ø> (ø)`	⬆️
...osoft.ML.Core.Tests/UnitTests/CoreBaseTestClass.cs	`68.78% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/Commands/TypeInfoCommand.cs	`0% <0%> (ø)`	⬆️
...s/StochasticDualCoordinateAscentClassifierBench.cs	`0% <0%> (ø)`	⬆️
....ML.Benchmarks/KMeansAndLogisticRegressionBench.cs	`0% <0%> (ø)`	⬆️
...c/Microsoft.ML.SamplesUtils/SamplesDatasetUtils.cs	`26.77% <0%> (ø)`	⬆️
...t/Microsoft.ML.Benchmarks/PredictionEngineBench.cs	`0% <0%> (ø)`	⬆️
... and 56 more

Ivanidzo4ka · 2019-02-22T01:41:03Z

Before reviewing this code two questions:

Are we happy with ScalarType name?
Do we ever plan to use this enum outside of TextLoader and TypeConverting? If no, I would suggest to have multiple ConvertToFloat, ConvertToDouble, ConvertToKey, etc to replace TypeConverting and just move that enum to TextLoader #Closed

wschin · 2019-02-22T06:31:41Z

@Ivanidzo4ka, your suggestion was one of the 3 solutions I have tried. Unfortunately, having specialized functions cannot tackle the case where multiple columns are casted to different types in one transformer. If you want to specialize ColumnInfo to different allowed types, you will end up with TWO groups of those ColumnInfo independently for TextLoader and TypeConverting. Therefore, @TomFinley and I feel it's ok to have a simplified version of DataKind. #Resolved

eerhardt · 2019-02-22T16:37:34Z

Are we happy with ScalarType name?

I had a similar concern initially. I tried to think of a better name, and the only name I thought was better was DataKind. What do you think about renaming the existing DataKind to InternalDataKind (or anything but DataKind), and then rename public enum ScalarType to public enum DataKind? #Resolved

wschin · 2019-02-22T17:07:15Z

Are we happy with ScalarType name?

I had a similar concern initially. I tried to think of a better name, and the only name I thought was better was DataKind. What do you think about renaming the existing DataKind to InternalDataKind (or anything but DataKind), and then rename public enum ScalarType to public enum DataKind?

Having a new name sounds reasonable to me. #Resolved

eerhardt · 2019-02-22T21:36:43Z

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

    /// </summary>
+    /// <remarks>
+    /// <list type="bullet">
+    /// <item><description><see cref="SByte"/>: 1-byte integer, type of <see cref="System.SByte"/>.</description></item>


Don't these comments typically go on each individual value in the enum?

public enum ActionType { /// <summary>The New Subscription is called</summary> Insert, /// <summary>The Cancel Subscription is called</summary> Update, /// <summary>The Cancel Subscription is called</summary> Cancel, /// <summary>The Get Subscription is called</summary> Retrieve }

#Resolved

eerhardt · 2019-02-22T21:37:32Z

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

@@ -8,40 +8,83 @@
 namespace Microsoft.ML.Data
 {
    /// <summary>
-    /// Data type specifier.
+    /// Data type specifier used in text loader and type converters.


I don't really like giving where a type is used as part of its definition/summary of what it is. What happens if a new place starts using this enum? #Resolved

Then we need to update this doc string. We have this new enum only because of the two classes mentioned. If we don't usually have design doc for each component, my feeling is that we should somehow describe the motivation of having an object here.

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

We have this new enum only because of the two classes mentioned.

Right now, sure. But nothing is stopping some new API from using this enum. Then this doc would be out of date. Saying where something is used now doesn't explain what this thing is.

How about simply:

Specifies a simple data type.

other enum summaries are pretty simple:

https://docs.microsoft.com/en-us/dotnet/api/system?view=netframework-4.7.2#enums

TypeCode | Specifies the type of an object.
DayOfWeek | Specifies the day of the week.

If you want to add where it is used, that would be better in a <remarks> section. #Resolved

Ok. I will move it to //.

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

eerhardt · 2019-02-22T21:41:05Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

+            /// <see cref="DataKind"/> of the items in the column.
+            /// </summary>
+            /// It's a public interface to access the information in an internal DataKind.
+            public DataKind ItemType => Type.HasValue ? Type.Value.ToScalarType() : DataKind.Single;


ToScalarType()? #Resolved

Will rename it to ToDataKind.

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

eerhardt · 2019-02-22T21:42:10Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

            /// <param name="source">Source index range(s) of the column.</param>
            /// <param name="keyCount">For a key column, this defines the range of values.</param>
-            public Column(string name, DataKind? type, Range[] source, KeyCount keyCount = null)
+            private Column(string name, InternalDataKind? kind, Range[] source, KeyCount keyCount = null)


Who calls this constructor? #Resolved

Only the three public constructors above.

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

eerhardt · 2019-02-22T21:42:21Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

+            {
+            }
+
+            public Column(string name, DataKind dataKind, Range[] source, KeyCount keyCount = null)


public API should have XML doc. #Resolved

eerhardt · 2019-02-22T21:43:21Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

+            /// <see cref="DataKind"/> of the items in the column.
+            /// </summary>
+            /// It's a public interface to access the information in an internal DataKind.
+            public DataKind ItemType => Type.HasValue ? Type.Value.ToScalarType() : DataKind.Single;


Why ItemType? Typically we use that name for the type of items inside of something - like a Vector. Why not just Type as the public name? #Resolved

We can't. Internal name, nameof(Type), is used in naming command line arguments.

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

I'm not sure I understand this reasoning. Can you explain further?

In general, we shouldn't be sacrificing our public API for "command line" reasonings. #Resolved

We use nameof(Type) as an argument name in entry point (classical APIs) and command line. If we rename Type to, for example, InternalType, old applications may be broken.

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

I'm still not convinced.... We are breaking all sorts of old applications with the API changes we've been making. We should define our public API as we want, and the other things will need to change.

@TomFinley - thoughts here? #Resolved

TextLoader is a widely used class. Braking BC may cost a lot. Also, Type is not very accurate. When we load a float vector column, its type will still be Float instead of Float[].

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

Also, we have this design guideline:

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/constructor

✓ DO use the same name for constructor parameters and a property if the constructor parameters are used to simply set the property.

The only difference between such parameters and the properties should be casing.

So this name should be in sync with the constructor parameter name. Maybe that would solve all your problems?

public DataKind DataKind to match the constructor parameter name: public Column(string name, DataKind dataKind, ...
#Resolved

TextLoader is a widely used class.

Agreed. That is why it is critical we get the API right. We can never change it after this. #Resolved

Ok. Let's use DataKind.

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

eerhardt · 2019-02-22T21:45:03Z

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

@@ -539,7 +540,7 @@ public sealed class ColumnInfo
            /// <summary>
            /// The expected kind of the converted column.
            /// </summary>
-            public readonly DataKind OutputKind;
+            internal readonly DataKind OutputKind;


Why is this internal? #Resolved

It shouldn't be. Thanks.

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

eerhardt · 2019-02-22T21:50:56Z

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

@@ -243,7 +243,7 @@ public void TypeConvertKeyBackCompatTest()
        {
            // Model generated using the following command before the change removing Min and Count from KeyType.
            // ML.Transforms.Conversion.ConvertType(new[] { new TypeConvertingEstimator.ColumnInfo("key", "convertedKey",
-            //      DataKind.U8, new KeyCount(4)) }).Fit(dataView);
+            //      ScalarType.Ulong, new KeyCount(4)) }).Fit(dataView);


ScalarType ? #Resolved

Should be DataKind.UInt64. Thanks.

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

eerhardt · 2019-02-22T22:50:44Z

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

    public enum DataKind : byte
    {
+        /// <summary><see cref="SByte"/>: 1-byte integer, type of <see cref="System.SByte"/>.</summary>


You don't need <see cref in the summary for the thing you are providing a summary for:

/// <summary>1-byte integer, type of <see cref="System.SByte"/>.</summary> ``` #Resolved

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

eerhardt · 2019-02-25T18:16:13Z

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

@@ -55,50 +95,67 @@ public enum DataKind : byte
    [BestFriend]
    internal static class DataKindExtensions


(nit) - maybe this could be InternalDataKindExtensions? #Resolved

Sure.

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

eerhardt · 2019-02-25T18:18:51Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs

+            /// <see cref="Data.DataKind"/> of the items in the column.
+            /// </summary>
+            /// It's a public interface to access the information in an internal DataKind.
+            public DataKind DataKind => Type.ToDataKind();


I think this should be settable. All the other fields are settable on this class:

Name, KeyCount, Source, but this one isn't. So if I use the default constructor - there is no way to set this property. #Resolved

Ok. Will make it

public DataKind DataKind { get { return Type.ToDataKind(); } set { Type = value.ToInternalDataKind(); } }

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

eerhardt

This is looking really good @wschin. I just have the one comment on the property setter. Once that is resolved, I can sign off.

eerhardt

Thanks @wschin !

Ivanidzo4ka

wschin self-assigned this Feb 20, 2019

TomFinley reviewed Feb 20, 2019

View reviewed changes

wschin force-pushed the internalize-datakind branch 2 times, most recently from 929df77 to 0fc552c Compare February 21, 2019 21:46

wschin changed the title ~~[WIP] Internalize DataKind~~ Internalize DataKind Feb 21, 2019

wschin requested review from eerhardt and Ivanidzo4ka February 21, 2019 23:38

wschin requested a review from TomFinley February 22, 2019 00:41

eerhardt reviewed Feb 22, 2019

View reviewed changes

wschin force-pushed the internalize-datakind branch from f4cedc6 to 3803bec Compare February 22, 2019 22:13

eerhardt reviewed Feb 22, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 22, 2019

View reviewed changes

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs Outdated Show resolved Hide resolved

eerhardt mentioned this pull request Feb 25, 2019

Make DataViewRowId not act like a number. #2707

Merged

wschin added 5 commits February 25, 2019 10:08

Hide the uses of DataKind in TypeConverting

afe3ee7

Hide DataKind used in TextLoader

ac7f822

Internalize-best-friend DataKind

c7f5880

DataKind ---> InternalDataKind

93d3ffe

ScalarType ---> DataKind (massive renaming)

7f28030

wschin added 6 commits February 25, 2019 10:09

Address comments

70b5936

Address comments

c767628

Address comments

9a803a0

Make R4 as default

e9fd4ae

Ok. I updated entry point...

9d1e49c

Sync with new things from master

0057fa6

eerhardt reviewed Feb 25, 2019

View reviewed changes

Address comments

02e4374

wschin force-pushed the internalize-datakind branch from 11b567d to 02e4374 Compare February 25, 2019 19:20

eerhardt approved these changes Feb 25, 2019

View reviewed changes

wschin requested a review from Ivanidzo4ka February 25, 2019 20:59

Ivanidzo4ka approved these changes Feb 25, 2019

View reviewed changes

wschin merged commit 2ef0614 into dotnet:master Feb 25, 2019

wschin deleted the internalize-datakind branch February 25, 2019 21:07

eerhardt mentioned this pull request Mar 21, 2019

Need to simplify Data Types to simpler/less common .NET types #1386

Closed

eerhardt mentioned this pull request Aug 29, 2019

Adding basic support for handling vectors in the database loader. #4138

Merged

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

		@@ -55,50 +95,67 @@ public enum DataKind : byte
		[BestFriend]
		internal static class DataKindExtensions

Internalize DataKind #2661

Internalize DataKind #2661

Uh oh!

Conversation

wschin commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Feb 20, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Feb 21, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Ivanidzo4ka commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wschin commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wschin commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Feb 22, 2019 • edited by wschin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wschin commented Feb 20, 2019 •

edited

Loading

TomFinley Feb 20, 2019 •

edited by wschin

Loading

wschin Feb 20, 2019 •

edited

Loading

TomFinley Feb 21, 2019 •

edited by wschin

Loading

wschin Feb 21, 2019 •

edited

Loading

codecov bot commented Feb 22, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 22, 2019 •

edited

Loading

wschin commented Feb 22, 2019 •

edited

Loading

eerhardt commented Feb 22, 2019 •

edited by wschin

Loading

wschin commented Feb 22, 2019 •

edited

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

wschin Feb 22, 2019 •

edited

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 22, 2019 •

edited by wschin

Loading

eerhardt Feb 25, 2019 •

edited by wschin

Loading

eerhardt Feb 25, 2019 •

edited by wschin

Loading