Make text loaders consistent #2710

wschin · 2019-02-25T18:04:22Z

Another attempt to fix #2472.

TomFinley · 2019-02-25T18:40:47Z

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

        /// <param name="dataSample">The optional location of a data sample. The sample can be used to infer column names and number of slots in each column.</param>
        public static TextLoader CreateTextLoader(this DataOperationsCatalog catalog,
            TextLoader.Column[] columns,
            char separatorChar = TextLoader.Defaults.Separator,
            bool hasHeader = TextLoader.Defaults.HasHeader,
            bool allowSparse = TextLoader.Defaults.AllowSparse,
            bool allowQuoting = TextLoader.Defaults.AllowQuoting,
+            bool trimWhitespace = TextLoader.Defaults.TrimWhitespace,
            IMultiStreamSource dataSample = null)


The data sample is really important. (See #2663.) If there are feature names, if there is a header, that's where they're coming from. It's really important. Burying it... what... eight levels deep seems a bit much. To bury it beyond such things as allowing sparsity and quoting itself seems really tough.

I might say the following order is most sensible:

Columns,

Separator char,

Has header,

Data sample,

Quoting,

Trimming,

Sparsity.

I might even think that 5, 6, and 7 could be relegated to advanced options, but no need to insist.

Just got them reordered. Thanks for sharing the list!

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

I think that if hasHeader is true, then the dataSample should not be allowed to be null (because if it is, there can be no slot names). Does it make sense to check this? Another alternative is instead of having a bool argument, to have an IMultiStreamSource header instead. But on the other hand, if there are two IMultiStreamSources that might be confusing. Not sure what the right signature is here. @Ivanidzo4ka, @TomFinley, what do you think?

In reply to: 260017117 [](ancestors = 260017117,259963185)

I think that if hasHeader is true, then the dataSample should not be allowed to be null (because if it is, there can be no slot names). Does it make sense to check this? Another alternative is instead of having a bool argument, to have an IMultiStreamSource header instead. But on the other hand, if there are two IMultiStreamSources that might be confusing. Not sure what the right signature is here. @Ivanidzo4ka, @TomFinley, what do you think?

In reply to: 260017117 [](ancestors = 260017117,259963185)

So, let's imagine that I declare that there is a header, but I provide no sample. I think this is fine. I am merely describing how to read data, and I am saying that when you read the data with this thing, that you will have a header to contend with. If headers were always only used for defining slot names, I might agree, but sometimes files have headers just because they have headers, and there's no larger reason for it.

The presence of absence of a sample does not mean that we cannot fully define the schema, even if header is true. (This is in contrast to some other options, like, "this is fixed size, but I would like you to auto-determine what that size is, which obviously requires a sample.)

So no check would be my reaction.

TomFinley · 2019-02-25T18:41:14Z

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

        /// <param name="allowQuoting">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.</param>
-        /// <param name="allowSparse">Whether the input may include sparse representations for example,
-        /// if one of the row contains "5 2:6 4:3" that's mean there are 5 columns all zero
-        /// except for 3rd and 5th columns which have values 6 and 3</param>
        /// <param name="trimWhitespace">Remove trailing whitespace from lines</param>
        public static TextLoader CreateTextLoader<TInput>(this DataOperationsCatalog catalog,


CreateTextLoader [](start = 33, length = 16)

This is not consistent. The file is missing altogether, which means it cannot be used to read slot names. Whoops.

Ok. I wrongly thought TInput can be used to encode SlotNames. IMultiStreamSource dataSample = null is just added.

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

TomFinley · 2019-02-25T18:42:01Z

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

        internal TextLoader(IHostEnvironment env, Column[] columns, char separatorChar = Defaults.Separator,
            bool hasHeader = Defaults.HasHeader, bool allowSparse = Defaults.AllowSparse,
-            bool allowQuoting = Defaults.AllowQuoting, IMultiStreamSource dataSample = null)
-            : this(env, MakeArgs(columns, hasHeader, new[] { separatorChar }, allowSparse, allowQuoting), dataSample)
+            bool allowQuoting = Defaults.AllowQuoting, IMultiStreamSource dataSample = null, bool trimWhitespace = Defaults.TrimWhitespace)


IMultiStreamSource dataSample = null [](start = 55, length = 36)

If you wanted you could just get rid of this constructor opportunistically. It serves no purpose as far as I can tell. #Resolved

Dropped.

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

artidoro · 2019-02-25T19:13:37Z

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

@@ -47,71 +49,78 @@ public static class TextLoaderSaverCatalog
        /// <param name="catalog">The <see cref="DataOperationsCatalog"/> catalog.</param>
        /// <param name="separatorChar">Column separator character. Default is '\t'</param>
        /// <param name="hasHeader">Does the file contains header?</param>
+        /// <param name="allowSparse">Whether the input may include sparse representations for example,
+        /// if one of the row contains "5 2:6 4:3" that's mean there are 5 columns all zero
+        /// except for 3rd and 5th columns which have values 6 and 3</param>
        /// <param name="allowQuoting">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.</param>


If you could make the comments consistent as well it would be great.
#Resolved

Sure.

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

artidoro

codecov · 2019-02-25T23:41:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f6d55f3). Click here to learn what that means.
The diff coverage is 50.8%.

@@            Coverage Diff            @@
##             master    #2710   +/-   ##
=========================================
  Coverage          ?   71.66%           
=========================================
  Files             ?      808           
  Lines             ?   142292           
  Branches          ?    16139           
=========================================
  Hits              ?   101972           
  Misses            ?    35879           
  Partials          ?     4441

Flag	Coverage Δ
#Debug	`71.66% <50.8%> (?)`
#production	`67.92% <95.74%> (?)`
#test	`85.82% <23.37%> (?)`

Impacted Files	Coverage Δ
...t/Microsoft.ML.Benchmarks/PredictionEngineBench.cs	`0% <0%> (ø)`
...s/StochasticDualCoordinateAscentClassifierBench.cs	`0% <0%> (ø)`
.../Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs	`81.71% <100%> (ø)`
...osoft.ML.Functional.Tests/Datasets/TypeTestData.cs	`96.34% <100%> (ø)`
test/Microsoft.ML.Functional.Tests/Prediction.cs	`100% <100%> (ø)`
...ML.Data/Transforms/ValueToKeyMappingTransformer.cs	`78.21% <100%> (ø)`
...ML.Tests/TrainerEstimators/MetalinearEstimators.cs	`100% <100%> (ø)`
test/Microsoft.ML.Tests/TextLoaderTests.cs	`98.9% <100%> (ø)`
...soft.ML.Functional.Tests/Datasets/MnistOneClass.cs	`83.33% <100%> (ø)`
test/Microsoft.ML.Functional.Tests/Evaluation.cs	`100% <100%> (ø)`
... and 3 more

TomFinley

Thanks @wschin this looks like it's in a much better place now!

singlis · 2019-02-26T19:31:33Z

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

@@ -47,79 +61,98 @@ public static class TextLoaderSaverCatalog
        /// <param name="catalog">The <see cref="DataOperationsCatalog"/> catalog.</param>
        /// <param name="separatorChar">Column separator character. Default is '\t'</param>
        /// <param name="hasHeader">Does the file contains header?</param>


hasHeader [](start = 25, length = 9)

Nit pick - I would rephrase this as "Set to true if the file contains a header row".

TomFinley · 2019-02-26T19:32:08Z

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

-        public static IDataView ReadFromTextFile(this DataOperationsCatalog catalog, string path, TextLoader.Options options = null)
+        /// <param name="dataSample">The optional location of a data sample. The sample can be used to infer column names and number of slots in each column.</param>
+        public static IDataView ReadFromTextFile(this DataOperationsCatalog catalog, string path,
+            TextLoader.Options options = null, IMultiStreamSource dataSample = null)


, IMultiStreamSource dataSample = null [](start = 45, length = 38)

Hi @wschin, somehow I missed this -- definitely we don't need to have a data sample if we're talking about the path to a data file already.

TomFinley · 2019-02-26T19:32:30Z

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

        /// <returns>The data view.</returns>
        public static IDataView ReadFromTextFile<TInput>(this DataOperationsCatalog catalog,
            string path,
            char separatorChar = TextLoader.Defaults.Separator,
            bool hasHeader = TextLoader.Defaults.HasHeader,
+            IMultiStreamSource dataSample = null,


IMultiStreamSource dataSample [](start = 12, length = 29)

Similar here.

singlis · 2019-02-26T19:35:52Z

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

+            bool trimWhitespace = TextLoader.Defaults.TrimWhitespace,
+            bool allowSparse = TextLoader.Defaults.AllowSparse)
+            => TextLoader.CreateTextReader<TInput>(CatalogUtils.GetEnvironment(catalog), hasHeader, separatorChar, allowQuoting,
+                allowSparse, trimWhitespace, dataSample: dataSample);


dataSample [](start = 45, length = 10)

why pass the arguments this way? Why not add dataSample after hasHeader?

singlis · 2019-02-26T19:38:50Z

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

        /// <param name="allowQuoting">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.</param>
+        /// <param name="trimWhitespace">Remove trailing whitespace from lines</param>


minor nit - Add a period.

wschin added the API Issues pertaining the friendly API label Feb 25, 2019

wschin self-assigned this Feb 25, 2019

wschin requested review from TomFinley and artidoro February 25, 2019 18:04

TomFinley reviewed Feb 25, 2019

View reviewed changes

artidoro reviewed Feb 25, 2019

View reviewed changes

wschin added 3 commits February 25, 2019 13:53

Make text loaders consistent

8ce91db

Address comments

ebe7ca8

Remove a redundant ctor of TextLoader

96d561e

wschin force-pushed the consistent-text-loaders branch from 55cde9e to 96d561e Compare February 25, 2019 21:59

artidoro approved these changes Feb 25, 2019

View reviewed changes

Fix tests

ead2583

TomFinley approved these changes Feb 26, 2019

View reviewed changes

wschin merged commit c90fa51 into dotnet:master Feb 26, 2019

wschin deleted the consistent-text-loaders branch February 26, 2019 19:29

singlis reviewed Feb 26, 2019

View reviewed changes

TomFinley reviewed Feb 26, 2019

View reviewed changes

singlis reviewed Feb 26, 2019

View reviewed changes

wschin mentioned this pull request Feb 26, 2019

Remove IMultiStreamSource when path (type: string) exists in text loader APIs #2745

Merged

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

Make text loaders consistent #2710

Make text loaders consistent #2710

wschin commented Feb 25, 2019

TomFinley Feb 25, 2019

wschin Feb 25, 2019

yaeldekel Feb 26, 2019

TomFinley Feb 26, 2019

TomFinley Feb 25, 2019

wschin Feb 25, 2019 •

edited

Loading

TomFinley Feb 25, 2019 •

edited by wschin

Loading

wschin Feb 25, 2019

artidoro Feb 25, 2019 •

edited by wschin

Loading

wschin Feb 25, 2019

artidoro left a comment

codecov bot commented Feb 25, 2019 •

edited

Loading

TomFinley left a comment

singlis Feb 26, 2019

TomFinley Feb 26, 2019

TomFinley Feb 26, 2019

singlis Feb 26, 2019

singlis Feb 26, 2019

Make text loaders consistent #2710

Make text loaders consistent #2710

Conversation

wschin commented Feb 25, 2019

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

Choose a reason for hiding this comment

wschin Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

TomFinley Feb 25, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Feb 25, 2019 • edited by wschin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Feb 25, 2019 •

edited

Loading

TomFinley Feb 25, 2019 •

edited by wschin

Loading

artidoro Feb 25, 2019 •

edited by wschin

Loading

codecov bot commented Feb 25, 2019 •

edited

Loading