Converted Schema to a class #1167

Zruty0 · 2018-10-05T21:12:28Z

Created a Schema class for eager schema.
Converted existing row mappers to use Schema.

Fixes #764

Converted existing row mappers to use Schema.

TomFinley · 2018-10-05T21:16:35Z

src/Microsoft.ML.Api/MapTransform.cs


 using System;
 using System.IO;
+using System.Linq;


Was this leftover from something? I don't see that LINQ gets used anywhere here. #Resolved

TomFinley · 2018-10-05T21:17:25Z

src/Microsoft.ML.Api/StatefulFilterTransform.cs


 using System;
 using System.IO;
+using System.Linq;


Similar, I guess perhaps you were doing something, then chose to do it a different way? #Resolved

…a-clean

Zruty0 · 2018-10-10T17:57:15Z

// Licensed to the .NET Foundation under one or more agreements.

significant change 1

Refers to: src/Microsoft.ML.Core/Data/Schema.cs:1 in 95daa3d. [](commit_id = 95daa3d, deletion_comment = False)

Zruty0 · 2018-10-10T17:57:37Z

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

+    /// <summary>
+    /// A logical row. Every value of every column is retrievable, and immutable.
+    /// </summary>
+    public interface IStandaloneRow : ISchematized


IStandaloneRow [](start = 21, length = 14)

significant change 2 #Closed

Actually not any more

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

Zruty0 · 2018-10-10T17:58:28Z

// Licensed to the .NET Foundation under one or more agreements.

significant change 3

Refers to: src/Microsoft.ML.Data/DataView/RowToRowMapperTransform.cs:1 in 95daa3d. [](commit_id = 95daa3d, deletion_comment = False)

Zruty0 · 2018-10-10T17:58:55Z

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

        }
    }

+    /// <summary>


summary [](start = 9, length = 7)

significant change 4

sfilipi · 2018-10-11T04:57:59Z

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

+#pragma warning disable CS0618 // Type or member is obsolete
+                              /// <summary>
+                              /// Manufacture an instance of <see cref="Schema"/> out of any <see cref="ISchema"/>.
+                              /// </summary>


indenting #Resolved

TomFinley · 2018-10-11T17:13:38Z

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

+namespace Microsoft.ML.Runtime.Data
+{
+#pragma warning disable CS0618 // Type or member is obsolete
+    public sealed class Schema : ISchema


Schema [](start = 24, length = 6)

Are you planning on writing actual documentation for this eventually? #Resolved

Will do right now

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

TomFinley · 2018-10-11T17:15:47Z

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

+        /// Puts a value of a column <paramref name="col"/> into <paramref name="value"/>.
+        /// This throws if the type <typeparamref name="TValue"/> differs from this column's type.
+        /// </summary>
+        void GetValue<TValue>(int col, ref TValue value);


GetValue(int col, ref TValue value); [](start = 13, length = 44)

I suppose that it would be a bug for GetGetter<>(col)(ref val to result in a separate value from GetValue<>(col ref val).

This strongly suggests that it would be better done via an extension method, rather than an implicit interface method. Otherwise we're just putting a bobby trap into our code.

Not sure which direction we want the extension method to go -- seems like GetValue as an extension might be cleaner, but I don't insist on this. #Resolved

If separate implementations are somehow useful, might be useful to know why.

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

I think GetValue is the one I'd prefer to keep. GetGetter is useful for 'recombining' rows into other rows, but it doesn't add to the consumption layer.

In reply to: 224532997 [](ancestors = 224532997,224531809)

Actually, I will for now remove the entire interface :) MetadataRow is the only implementation.

In reply to: 224543998 [](ancestors = 224543998,224532997,224531809)

TomFinley · 2018-10-11T17:58:12Z

I'm done looking :D #Closed

…earning into feature/new-schema-clean

Zruty0 · 2018-10-11T18:22:26Z

Time to look again! or approve! It's Hacktober after all

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

TomFinley · 2018-10-12T15:52:44Z

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

+        /// <summary>
+        /// The metadata of one <see cref="Column"/>.
+        /// </summary>
+        public sealed class MetadataRow


MetadataRow [](start = 28, length = 11)

You removed the interface, which is fine. But do you still want it to be ISchematized? #Resolved

Also the name "MetadataRow"... it is row-y in the sense that it is a collection of columns, but otherwise it doesn't seem very row-y somehow.

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

Renamed to Metadata

In reply to: 224835091 [](ancestors = 224835091,224834244)

TomFinley

Looks like a good forward step, thanks @Zruty0.

yaeldekel · 2018-10-12T16:35:07Z

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

+            /// <summary>
+            /// Get the value of the metadata, by column index.
+            /// </summary>
+            public void GetValue<TValue>(int col, ref TValue value) => GetGetter<TValue>(col)(ref value);


GetGetter(col) [](start = 71, length = 22)

Would this throw if TValue is wrong? #Resolved

Yes

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

It should at least

In reply to: 224847616 [](ancestors = 224847616,224846502)

yaeldekel · 2018-10-12T16:35:55Z

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

+            /// <summary>
+            /// Get a getter delegate for one value of the metadata row.
+            /// </summary>
+            public ValueGetter<TValue> GetGetter<TValue>(int col)


GetGetter [](start = 39, length = 9)

Does this method (and the next one) have to be public? #Resolved

GetGetter does. GetValue by col didn't

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

…a-clean

Ivanidzo4ka · 2018-10-15T23:42:37Z

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

+            /// </summary>
+            public Metadata Metadata { get; }
+
+            public Column(string name, ColumnType type, Metadata metadata)


metadata [](start = 65, length = 8)

any reason why you don't want to have default nullable here?

I was thinking about it. For now, let's keep it as is and observe some more usage patterns.

Ivanidzo4ka

Created a Schema class for eager schema.

9e19963

Converted existing row mappers to use Schema.

Zruty0 added the API Issues pertaining the friendly API label Oct 5, 2018

Zruty0 self-assigned this Oct 5, 2018

Zruty0 requested a review from TomFinley October 5, 2018 21:12

TomFinley requested a review from yaeldekel October 5, 2018 21:13

TomFinley reviewed Oct 5, 2018

View reviewed changes

Pete Luferenko added 7 commits October 5, 2018 18:07

Merge

a00091a

Merge remote-tracking branch 'upstream/master' into feature/new-schem…

9e5dbe7

…a-clean

Fixing the build after another merge

901dba9

Another round of ISchema cleanup

b27d7c4

Fixed some tests

b6573dd

Fixed someone's bug in LabelConvert

031f0f3

Merge remote-tracking branch 'upstream/master' into feature/new-schem…

95daa3d

…a-clean

Zruty0 commented Oct 10, 2018

View reviewed changes

Zruty0 mentioned this pull request Oct 10, 2018

Debugger views of estimators #1222

Closed

sfilipi reviewed Oct 11, 2018

View reviewed changes

TomFinley reviewed Oct 11, 2018

View reviewed changes

Pete Luferenko added 3 commits October 11, 2018 11:04

PR comments

2f8bfa8

Merge branch 'feature/new-schema-clean' of github.com:Zruty0/machinel…

ebe3178

…earning into feature/new-schema-clean

Merge

182c9be

TomFinley reviewed Oct 12, 2018

View reviewed changes

TomFinley approved these changes Oct 12, 2018

View reviewed changes

yaeldekel reviewed Oct 12, 2018

View reviewed changes

Pete Luferenko added 4 commits October 12, 2018 10:10

PR comments

fd9fb6f

Merge remote-tracking branch 'upstream/master' into feature/new-schem…

6ce1d0c

…a-clean

Proper rename of classes

ce144ed

Merge remote-tracking branch 'upstream/master' into feature/new-schem…

94c8e13

…a-clean

singlis mentioned this pull request Oct 15, 2018

Replace DropColumns,KeepColumns and ChooseColumns with SelectColumns #1269

Merged

Ivanidzo4ka reviewed Oct 15, 2018

View reviewed changes

Ivanidzo4ka approved these changes Oct 15, 2018

View reviewed changes

Zruty0 merged commit 8e77d0f into dotnet:master Oct 16, 2018

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

Converted Schema to a class #1167

Converted Schema to a class #1167

Uh oh!

Conversation

Zruty0 commented Oct 5, 2018

Uh oh!

TomFinley Oct 5, 2018 • edited by Zruty0 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 Oct 5, 2018 • edited by Zruty0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 commented Oct 10, 2018

Uh oh!

Zruty0 Oct 10, 2018 • 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!

Zruty0 commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Oct 11, 2018 • edited by Zruty0 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 Oct 11, 2018 • edited by Zruty0 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!

TomFinley Oct 11, 2018 • edited by Zruty0 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Oct 11, 2018 • edited by Zruty0 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zruty0 commented Oct 11, 2018

Uh oh!

TomFinley Oct 12, 2018 • edited by Zruty0 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!

Choose a reason for hiding this comment

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

yaeldekel Oct 12, 2018 • edited by Zruty0 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!

Choose a reason for hiding this comment

Uh oh!

yaeldekel Oct 12, 2018 • edited by Zruty0 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley Oct 5, 2018 •

edited by Zruty0

Loading

TomFinley Oct 5, 2018 •

edited by Zruty0

Loading

Zruty0 Oct 10, 2018 •

edited

Loading

sfilipi Oct 11, 2018 •

edited by Zruty0

Loading

TomFinley Oct 11, 2018 •

edited by Zruty0

Loading

TomFinley Oct 11, 2018 •

edited by Zruty0

Loading

TomFinley commented Oct 11, 2018 •

edited by Zruty0

Loading

TomFinley Oct 12, 2018 •

edited by Zruty0

Loading

yaeldekel Oct 12, 2018 •

edited by Zruty0

Loading

yaeldekel Oct 12, 2018 •

edited by Zruty0

Loading