Any reason for inconsistency in params for TextLoader?

[srsaggam]

1. This Method :
   

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

   Line 55 in 834e471

   public static TextLoader CreateTextLoader<TInput>(this DataOperationsCatalog catalog,

2. and this one :
   

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

   Line 22 in 834e471

   public static TextLoader CreateTextLoader(this DataOperationsCatalog catalog,

Looks like the first one has params like :

char separatorChar = '\t', bool allowQuotedStrings = true, bool supportSparse = true, bool trimWhitespace = false

Where as the second one doesn't have those. In order to use those params there is another method overload :

CreateTextLoader(this DataOperationsCatalog catalog, TextLoader.Arguments args, IMultiStreamSource dataSample = null);

This takes Arguments class and particularly one param is interesting in this Arguments:

char [] Separators

Any reason this is char[] and not char like in other overloads? and why inconsistency in params for the methods 1) and 2).

[srsaggam]

@daholste @justinormont

[TomFinley]

@rogancarr and @sfilipi, should be made consistent.

[abgoswam]

@TomFinley . Could you kindly clarify what you mean by should be made consistent ?

To me it seems reasonable that the "simple" APIs (1 and 2 above) take a char to indicate a single separator character.

If a user wants to specify multiple separator characters they can use the "advanced" API which takes in the Options

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

Lines 39 to 41 in eecf272

	public static TextLoader CreateTextLoader(this DataOperationsCatalog catalog,
	TextLoader.Options options,
	IMultiStreamSource dataSample = null)

cc @wschin @artidoro

[TomFinley]

Yes @abgoswam I agree, the convenience methods should continue to take char. The advanced options should still take char[] I think, since it is sometimes important.

Of particular interest to me was the lack of these:

bool allowQuotedStrings = TextLoader.DefaultArguments.AllowQuoting,
bool supportSparse = TextLoader.DefaultArguments.AllowSparse,
bool trimWhitespace = TextLoader.DefaultArguments.TrimWhitespace)

It is not clear to me that the overloads should differ in this respect. Also why does one have the possibility of handling a file handle (which enables it to read feature names into the schema) and the other does not?

So these two overloads are inconsistent leading to loss of capability in one or the other, and I think this should be made consistent.

[rogancarr]

Also, see #2512: We only support a limited set of characters for saving a text file, so perhaps we should move away from an explicit char and use an enum of allowed characters instead.