Textloader internalizations, documentation, and Arguments refactoring #2417

artidoro · 2019-02-05T06:16:54Z

Fixes #2046.
Related to #1798.

This PR does the following:

Addresses the problem raised by TextLoader Arguments/ArgumentsCore classes #2046 by creating a single Arguments class from the original two classes Arguments and ArgumentsCore. The issue suggested the first approach out of two possible ways to do this, which is the one I followed in this PR.
Renames Arguments to Options.
Internalizes the constructors and other fields that should not be in the public API.
Adds comments for all public fields and methods.

codecov · 2019-02-05T07:12:15Z

Codecov Report

Merging #2417 into master will decrease coverage by 0.01%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #2417      +/-   ##
==========================================
- Coverage   71.55%   71.53%   -0.02%     
==========================================
  Files         800      800              
  Lines      141846   141847       +1     
  Branches    16119    16119              
==========================================
- Hits       101497   101474      -23     
- Misses      35893    35922      +29     
+ Partials     4456     4451       -5

Flag	Coverage Δ
#Debug	`71.53% <89.28%> (-0.02%)`	⬇️
#production	`67.82% <88.88%> (-0.03%)`	⬇️
#test	`85.73% <100%> (ø)`	⬆️

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

sfilipi · 2019-02-06T18:37:23Z

    ///     col=ColumnName:U4[100]:1,3-10

those are cmd examples. Maybe move them in regular comments above the column, but not XML comments. #Resolved

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:43 in f75b504. [](commit_id = f75b504, deletion_comment = False)

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

sfilipi · 2019-02-06T18:38:50Z

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

@@ -199,6 +237,9 @@ internal bool IsValid()
            }
        }

+        /// <summary>
+        /// Specifies the range of indices of input columns that should be mapped to an output column.
+        /// </summary>
        public sealed class Range
        {
            public Range() { }


public Range() { } [](start = 11, length = 19)

is this used? #Pending

It is! And I think you are right it looks a bit strange. After discussing with Tom, I am going to open an issue about reshaping the public surface of TextLoader and fix it separately. Let's keep this PR only for internalization and refactoring of Arguments.

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

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

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs

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

src/Microsoft.ML.Data/DataLoadSave/DataReaderExtensions.cs

src/Microsoft.ML.StaticPipe/LocalPathReader.cs

Ivanidzo4ka · 2019-02-08T17:32:25Z

artidoro force-pushed the artidoro:textloader branch from 7ff9724 to e3b17b9 2 days ago

I'm glad what you awere of git fu, but rebase and force push screw up codeflow, it get;s hard to track comments. Can you just merge and push?

On other note, do you plan to update this PR? #Resolved

justinormont · 2019-02-08T21:28:39Z

Related issue: #2472 "Any reason for inconsistency in params for TextLoader?" #Resolved

artidoro · 2019-02-09T02:48:54Z

I am still trying to learn from master Tom about git magic, but I understand the problem with rebase once the PR is out.

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

artidoro · 2019-02-09T02:50:17Z

Absolutely, I will address that and further concerns regarding TextLoader in another PR. This one is just to reduce the current public surface to what is essential. In that issue, and in a future PR I think we should rethink what we expose and possibly simplify it.

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

artidoro · 2019-02-09T03:12:01Z

I have resolved Ivan's comments, so if @Ivanidzo4ka and @sfilipi could take another look it would be great. #Resolved

src/Microsoft.ML.Data/DataLoadSave/DataReaderExtensions.cs

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

Ivanidzo4ka · 2019-02-12T05:14:35Z

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

+            /// Describes how an input column should be mapped to an <see cref="IDataView"/> column.
+            /// </summary>
+            /// <param name="name">Name of the column.</param>
+            /// <param name="type"><see cref="DataKind"/> of the items in the column. If <see langword="null"/> defaults to a <see cref="DataKind.R4"/>.</param>


If [](start = 86, length = 25)

Just a rant.
Only reason why we have it is CommandLine in which you can say something like `col=ColA:0" and expect it to work.
On programmatic level idea of "Hey its a nullable field but if you set it null we will treat it as float" is weird.
If you want to change it, one tricky place is TryParse method line 123, just set Type straight to DataKind.Num since in other cases it can be never setup.
#Pending

I agree it does not make much sense to have this as a nullable type. We should just have a default. I would like to address this in another PR as I mention in my other comment.

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

Ivanidzo4ka · 2019-02-12T05:16:24Z

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

@@ -334,66 +395,98 @@ internal bool TryUnparse(StringBuilder sb)
            }
        }

-        public class ArgumentsCore
+        /// <summary>
+        /// The settings for <see cref="TextLoader"/>


settings [](start = 16, length = 8)

options. #Resolved

Ivanidzo4ka · 2019-02-12T05:18:13Z

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

+            /// <summary>
+            /// Whether the input may include quoted values, which can contain separator characters, colons,
+            /// and distinguish empty values from missing values. When true, consecutive separators denote a
+            /// missing value and an empty value is denoted by \"\". When false, consecutive separators denote an empty value.


"" [](start = 63, length = 4)

where should be specific documentation magic to properly represent such strings. ` maybe? #Pending

See my comment, but let me know if you think otherwise.

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

Ivanidzo4ka · 2019-02-12T05:21:29Z

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


+            /// <summary>
+            /// Whether the input may include sparse representations.


Whether the input may include sparse representations. [](start = 16, length = 53)

[citation needed] (https://xkcd.com/285/)
not sure how it should look like, but we should specify how sparse format looks like. it's a index:value separated pairs where each index should be bigger than previous one. #Pending

I also not sure can we have multiple sparse columns or not.

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

I would like to do another PR focusing on improving the public surface of text loader. I think these are all important points, and I will come back to them to improve the documentation.
Before I focus on improving the documentation for these public fields I would like to make sure that we want to expose them all and that the current form is the final one.

In reply to: 255806866 [](ancestors = 255806866,255806745)

Ivanidzo4ka · 2019-02-12T05:24:10Z

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


+            /// <summary>
+            /// Number of source columns in the text data. Default is that sparse rows contain their size information.


Number [](start = 16, length = 6)

would be nice to have example
theoretically we can have sparse format like SparseSize separtor pairs of index:value devided by separator
743 1:2 5:-1 8:0.4 742:1 743:2
not sure what would happen if we have 744:7 in that line. #Pending

See my other comment regarding another PR after discussing the public surface of TextLoader.

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

Ivanidzo4ka · 2019-02-12T05:24:56Z

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


+            /// <summary>
+            /// The characters that should be used as separators column separator.


separators column separator [](start = 53, length = 28)

does it sound right for you? #Resolved

No, thank you for pointing out.

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

Ivanidzo4ka · 2019-02-12T05:31:04Z

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

+            string tmp = CmdParser.GetSettings(host, options, new Options()
+            {
+                // It's fine if the user sets the following three arguments that are not considered core arguments.
+                // Setting the defaults to the user provided values will avoid these in the output of the call CmdParser.GetSettings.


Why we override them here? Can you override them in upstream function? #Pending

GetSettings builds a string with the fields in the second parameter options that are different from the fields in the third parameter in this case new Options () {...}. Previously, only the core arguments were considered when taking the difference between the user provided arguments and the defaults. Since now there is no distinction between core and non core arguments, I achieve exactly the same behavior as before by setting the non core arguments in the third parameter to the user provided values.

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

artidoro · 2019-02-14T00:30:35Z

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

@@ -1145,7 +1238,14 @@ private sealed class LoaderHolder

            // Verify that the current schema-defining arguments are default.
            // Get settings just for core arguments, not everything.
-            string tmp = CmdParser.GetSettings(host, args, new ArgumentsCore());
+            string tmp = CmdParser.GetSettings(host, options, new Options()


Null check.

Ivanidzo4ka · 2019-02-14T19:18:56Z

You still have conflicting files :)

TomFinley

Hi @artidoro sorry for not looking at this sooner, I thought from the context that this was more a "public surface API" change moreso than anything architectural. Looks good, thank you.

abgoswam · 2019-02-19T23:19:40Z

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

                    goto LDone;

-                cols = argsNew.Columns;
+                // Set the non core arguments in the new options.


non core arguments [](start = 27, length = 18)

perhaps we should clarify what are "non core" arguments here ? would be especially useful for the future.

without #2046 i did not have enough context to know what this meant

abgoswam · 2019-02-19T23:20:58Z

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

        // If not, set error to true if there was an error condition.
        private static bool TryParseSchema(IHost host, IMultiStreamSource files,
            ref Options options, out Column[] cols, out bool error)
        {
            host.AssertValue(host);
            host.AssertValue(files);
+            host.CheckValue(options, nameof(options));

            cols = null;
            error = false;

            // Verify that the current schema-defining arguments are default.
            // Get settings just for core arguments, not everything.


core arguments [](start = 37, length = 14)

perhaps we should rephrase this , so it makes sense for future readers ?

abgoswam

artidoro added API Issues pertaining the friendly API documentation Related to documentation of ML.NET labels Feb 5, 2019

artidoro self-assigned this Feb 5, 2019

artidoro requested review from TomFinley, najeeb-kazmi, ganik and sfilipi February 5, 2019 06:16

sfilipi reviewed Feb 6, 2019

View reviewed changes

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

sfilipi reviewed Feb 6, 2019

View reviewed changes

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

sfilipi reviewed Feb 6, 2019

View reviewed changes

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

sfilipi reviewed Feb 6, 2019

View reviewed changes

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs Show resolved Hide resolved

sfilipi reviewed Feb 6, 2019

View reviewed changes

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

sfilipi reviewed Feb 6, 2019

View reviewed changes

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

Ivanidzo4ka reviewed Feb 7, 2019

View reviewed changes

src/Microsoft.ML.Data/DataLoadSave/DataReaderExtensions.cs Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 7, 2019

View reviewed changes

src/Microsoft.ML.StaticPipe/LocalPathReader.cs Show resolved Hide resolved

artidoro force-pushed the textloader branch from 7ff9724 to e3b17b9 Compare February 7, 2019 05:08

artidoro requested a review from singlis February 7, 2019 19:23

Ivanidzo4ka reviewed Feb 9, 2019

View reviewed changes

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

Ivanidzo4ka reviewed Feb 9, 2019

View reviewed changes

src/Microsoft.ML.Data/DataLoadSave/DataReaderExtensions.cs Show resolved Hide resolved

Ivanidzo4ka reviewed Feb 9, 2019

View reviewed changes

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

singlis reviewed Feb 10, 2019

View reviewed changes

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

Ivanidzo4ka reviewed Feb 12, 2019

View reviewed changes

artidoro commented Feb 14, 2019

View reviewed changes

artidoro force-pushed the textloader branch from 97dad92 to 7ff9724 Compare February 14, 2019 19:13

artidoro force-pushed the textloader branch from fad01cf to aa27507 Compare February 15, 2019 01:29

TomFinley approved these changes Feb 19, 2019

View reviewed changes

artidoro added 8 commits February 19, 2019 14:39

renamed arguments, added doc and internalized

bc7cb49

fixed TryParseSchema

a6b6d29

cosmetic change

8316bd6

add some documentation and cleaned up extensions

1b70723

review comments

c0e80a2

add option not null check

3265f01

merge fix

f37668f

fix merge

592e75c

artidoro force-pushed the textloader branch from aa27507 to 592e75c Compare February 19, 2019 22:49

artidoro requested a review from abgoswam February 19, 2019 22:51

abgoswam reviewed Feb 19, 2019

View reviewed changes

abgoswam approved these changes Feb 19, 2019

View reviewed changes

added comments to make the function more clear

a5cb965

artidoro merged commit a25763e into dotnet:master Feb 20, 2019

shauheen added this to the 0219 milestone Feb 21, 2019

artidoro deleted the textloader branch March 13, 2019 17:55

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Textloader internalizations, documentation, and Arguments refactoring #2417

Textloader internalizations, documentation, and Arguments refactoring #2417

artidoro commented Feb 5, 2019

codecov bot commented Feb 5, 2019 •

edited

Loading

sfilipi commented Feb 6, 2019 •

edited by artidoro

Loading

sfilipi Feb 6, 2019 •

edited by artidoro

Loading

artidoro Feb 7, 2019

Ivanidzo4ka commented Feb 8, 2019 •

edited by artidoro

Loading

justinormont commented Feb 8, 2019 •

edited by artidoro

Loading

artidoro commented Feb 9, 2019

artidoro commented Feb 9, 2019

artidoro commented Feb 9, 2019 •

edited

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

artidoro Feb 13, 2019

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

artidoro Feb 13, 2019

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019

artidoro Feb 13, 2019

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

artidoro Feb 13, 2019

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

artidoro Feb 13, 2019

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

artidoro Feb 13, 2019

artidoro Feb 14, 2019

Ivanidzo4ka commented Feb 14, 2019 •

edited

Loading

TomFinley left a comment

abgoswam Feb 19, 2019

abgoswam Feb 19, 2019

abgoswam left a comment


		/// <summary>
		/// Whether the input may include sparse representations.


		/// <summary>
		/// Number of source columns in the text data. Default is that sparse rows contain their size information.


		/// <summary>
		/// The characters that should be used as separators column separator.

Textloader internalizations, documentation, and Arguments refactoring #2417

Textloader internalizations, documentation, and Arguments refactoring #2417

Conversation

artidoro commented Feb 5, 2019

codecov bot commented Feb 5, 2019 • edited Loading

Codecov Report

sfilipi commented Feb 6, 2019 • edited by artidoro Loading

sfilipi Feb 6, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 8, 2019 • edited by artidoro Loading

justinormont commented Feb 8, 2019 • edited by artidoro Loading

artidoro commented Feb 9, 2019

artidoro commented Feb 9, 2019

artidoro commented Feb 9, 2019 • edited Loading

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 12, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 14, 2019 • edited Loading

TomFinley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abgoswam left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 5, 2019 •

edited

Loading

sfilipi commented Feb 6, 2019 •

edited by artidoro

Loading

sfilipi Feb 6, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 8, 2019 •

edited by artidoro

Loading

justinormont commented Feb 8, 2019 •

edited by artidoro

Loading

artidoro commented Feb 9, 2019 •

edited

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka Feb 12, 2019 •

edited by artidoro

Loading

Ivanidzo4ka commented Feb 14, 2019 •

edited

Loading