Clean up the SchemaDefinition class #2995

yaeldekel · 2019-03-18T13:26:23Z

Fixes #2978 .

yaeldekel · 2019-03-18T13:28:49Z

src/Microsoft.ML.Data/Data/SchemaDefinition.cs


-            public Column(IExceptionContext ectx, string memberName, DataViewType columnType,
-                string columnName = null, IEnumerable<AnnotationInfo> annotationInfos = null, Delegate generator = null)
+            public Column(string memberName, DataViewType columnType,


public [](start = 12, length = 6)

I would like to make this internal as well. Is there any scenario where users would want to add new columns to the SchemaDefinition generated from the type?

I think so. In fact I think that's one of the major scenarios, probably the central reason why people need this structure, isn't it? You start with an empty schema definition, then you add columns to it to describe how you want fields mapped to columns, etc.? This object exists in large part for those situations where you don't or somehow can't rely purely on the reflection based mechanism.

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

I was thinking you start with a schema that's auto-generated from the type, then modify the columns that are already in it. The schema generated from the type contains all the possible columns, why would we need to add new columns (as opposed to modifying existing columns)?

In reply to: 266605420 [](ancestors = 266605420,266437352)

codecov · 2019-03-18T14:05:53Z

Codecov Report

Merging #2995 into master will increase coverage by 0.02%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #2995      +/-   ##
==========================================
+ Coverage   72.41%   72.43%   +0.02%     
==========================================
  Files         803      804       +1     
  Lines      143851   143916      +65     
  Branches    16173    16173              
==========================================
+ Hits       104171   104250      +79     
+ Misses      35258    35250       -8     
+ Partials     4422     4416       -6

Flag	Coverage Δ
#Debug	`72.43% <90.47%> (+0.02%)`	⬆️
#production	`68.1% <67.85%> (+0.01%)`	⬆️
#test	`88.63% <98.7%> (+0.02%)`	⬆️

Impacted Files	Coverage Δ
test/Microsoft.ML.Tests/Scenarios/Api/TestApi.cs	`97.63% <100%> (+0.01%)`	⬆️
...soft.ML.Data/DataView/DataViewConstructionUtils.cs	`85.27% <100%> (-0.9%)`	⬇️
...osoft.ML.Data/DataView/InternalSchemaDefinition.cs	`56.94% <50%> (ø)`	⬆️
src/Microsoft.ML.Data/Data/SchemaDefinition.cs	`69.87% <71.42%> (+1.01%)`	⬆️
...osoft.ML.Functional.Tests/SchemaDefinitionTests.cs	`98.46% <98.46%> (ø)`
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	`89.26% <0%> (-0.63%)`	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	`85.78% <0%> (+0.15%)`	⬆️
...ML.Transforms/MutualInformationFeatureSelection.cs	`78.9% <0%> (+0.54%)`	⬆️
... and 4 more

eerhardt · 2019-03-18T14:41:41Z

test/Microsoft.ML.Functional.Tests/SchemaDefinitionTests.cs

+
+namespace Microsoft.ML.Functional.Tests
+{
+    public class PredictionEngineScenarios : BaseTestClass


(nit) I would name the file and the class name the same. #Resolved

eerhardt · 2019-03-18T14:43:47Z

test/Microsoft.ML.Functional.Tests/SchemaDefinitionTests.cs

+            base.Initialize();
+
+            _ml = new MLContext(42);
+            _ml.AddStandardComponents();


Do you need to call AddStandardComponents? That should only be necessary when you are doing things like using the MAML syntax. When you are strictly using the API, it shouldn't be necessary.

I added this line back, because of issue #2996.

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

eerhardt

TomFinley · 2019-03-18T19:31:37Z

src/Microsoft.ML.Data/Data/SchemaDefinition.cs

                }
            }
        }

+        private SchemaDefinition()


SchemaDefinition [](start = 16, length = 16)

So, why is this private? I'm thinking about how I'd like to use it. I have my class, I create a new schema definition (but empty), then I populate the mapping. Do I have any other way to create an empty one of these guys?

It appears that not only is there no way to create an empty one, but there is no longer a way to add a new column to it, since the column constructor is now internal.

Are we sure there are no scenarios that need to do this?

TomFinley · 2019-03-18T19:36:19Z

    public static SchemaDefinition Create(Type userType, Direction direction = Direction.Both)

Is this actually the only way to create a schema definition? As far as I see this auto-populates everything, which is sort of the opposite of what someone actually trying to use this structure would want to do, most of the time. (Since the entire reason someone wants to create this thing is to be explicit about the dataview-type mapping.)

Refers to: src/Microsoft.ML.Data/Data/SchemaDefinition.cs:326 in de633a3. [](commit_id = de633a3, deletion_comment = False)

TomFinley · 2019-03-18T19:36:33Z

    public static SchemaDefinition Create(Type userType, Direction direction = Direction.Both)

Maybe if you hadn't made the constructor private that would be sufficient.

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

Refers to: src/Microsoft.ML.Data/Data/SchemaDefinition.cs:326 in de633a3. [](commit_id = de633a3, deletion_comment = False)

TomFinley

Hi @yaeldekel thanks for working on this. The primary reason why this thing exists is to allow people to be explicit about the mapping of columns. While many of the internalizations and cleanups were appropriate, we took out the ability to construct an empty one, which is the primary use case. So we ought to add that back in.

TomFinley

Thanks @yaeldekel!

yaeldekel requested review from eerhardt, TomFinley and sfilipi March 18, 2019 13:26

yaeldekel commented Mar 18, 2019

View reviewed changes

eerhardt reviewed Mar 18, 2019

View reviewed changes

eerhardt approved these changes Mar 18, 2019

View reviewed changes

yaeldekel force-pushed the schemadefinition branch from 2023059 to de633a3 Compare March 18, 2019 18:26

TomFinley reviewed Mar 18, 2019

View reviewed changes

TomFinley suggested changes Mar 18, 2019

View reviewed changes

yaeldekel force-pushed the schemadefinition branch from 9e7dd80 to cbc96e7 Compare March 19, 2019 18:17

TomFinley approved these changes Mar 19, 2019

View reviewed changes

yaeldMS added 7 commits March 20, 2019 08:58

Internalize some members of SchemaDefinition, and add tests

1d2d219

Code review comments

f23cdbf

Fix build after rebase

be22be6

Fix failing test

b5cf7a4

Fix build after rebase

5ce0537

Internalize Column ctor

1860cce

Fix build after rebase

0598c9f

yaeldekel force-pushed the schemadefinition branch from fb6481b to 0598c9f Compare March 20, 2019 16:10

yaeldekel merged commit 807d813 into dotnet:master Mar 20, 2019

yaeldekel deleted the schemadefinition branch March 20, 2019 17:01

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up the SchemaDefinition class #2995

Clean up the SchemaDefinition class #2995

yaeldekel commented Mar 18, 2019

yaeldekel Mar 18, 2019

TomFinley Mar 18, 2019 •

edited

Loading

yaeldekel Mar 18, 2019

codecov bot commented Mar 18, 2019 •

edited

Loading

eerhardt Mar 18, 2019 •

edited by yaeldekel

Loading

eerhardt Mar 18, 2019 •

edited by yaeldekel

Loading

yaeldekel Mar 18, 2019

eerhardt left a comment

TomFinley Mar 18, 2019

eerhardt Mar 20, 2019

TomFinley commented Mar 18, 2019

TomFinley commented Mar 18, 2019

TomFinley left a comment

TomFinley left a comment

Clean up the SchemaDefinition class #2995

Clean up the SchemaDefinition class #2995

Conversation

yaeldekel commented Mar 18, 2019

yaeldekel Mar 18, 2019

Choose a reason for hiding this comment

TomFinley Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

yaeldekel Mar 18, 2019

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2019 • edited Loading

Codecov Report

eerhardt Mar 18, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

eerhardt Mar 18, 2019 • edited by yaeldekel Loading

Choose a reason for hiding this comment

yaeldekel Mar 18, 2019

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

TomFinley Mar 18, 2019

Choose a reason for hiding this comment

eerhardt Mar 20, 2019

Choose a reason for hiding this comment

TomFinley commented Mar 18, 2019

TomFinley commented Mar 18, 2019

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley Mar 18, 2019 •

edited

Loading

codecov bot commented Mar 18, 2019 •

edited

Loading

eerhardt Mar 18, 2019 •

edited by yaeldekel

Loading

eerhardt Mar 18, 2019 •

edited by yaeldekel

Loading