address code review comments and add more tests

yaeldMS · yaeldMS · commit 735826655211 · 2019-11-10T13:14:42.000+02:00
diff --git a/docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/LoadingSvmLight.cs b/docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/LoadingSvmLight.cs
@@ -76,7 +76,7 @@ public static void Example()
             // 1 1 0 0.02958358 0.9334617 0 0 0.8833956 0.2947684 0 0 0
 
             // If the loader is created without a data sample we need to specify the number of features expected in the file.
-            loader = mlContext.Data.CreateSvmLightLoader(10);
+            loader = mlContext.Data.CreateSvmLightLoader(inputSize: 10);
             svmData = loader.Load(file);
 
             PrintSchema(svmData);
diff --git a/src/Microsoft.ML.Transforms/SvmLight/SvmLightLoader.cs b/src/Microsoft.ML.Transforms/SvmLight/SvmLightLoader.cs
@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Collections;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -169,14 +170,9 @@ public void MapInput(Input input, IntermediateInput intermediate)
 
                 var ator = ReadOnlyMemoryUtils.Split(left, _seps).GetEnumerator();
 
-                if (!ator.MoveNext())
-                {
-                    intermediate.Label = float.NaN;
-                    intermediate.Weight = float.NaN;
-                    VBufferUtils.Clear(ref intermediate.FeatureKeys);
-                    VBufferUtils.Clear(ref intermediate.FeatureValues);
-                    return;
-                }
+                // Empty lines are filtered in the Input.MapComment step.
+                var notEmpty = ator.MoveNext();
+                Contracts.Assert(notEmpty);
 
                 ReadOnlyMemory<char> token = ator.Current;
 
@@ -239,6 +235,7 @@ public void MapInput(Input input, IntermediateInput intermediate)
                         // it a feature, but right now we have no learners that pay
                         // attention to so-called "slack IDs" so we'll ignore these for
                         // right now.
+                        continue;
                     }
                     else
                     {
@@ -321,10 +318,16 @@ public void ParseIndices(IntermediateInput input, Indices output)
                 var inputValues = input.FeatureKeys.GetValues();
                 for (int i = 0; i < inputValues.Length; i++)
                 {
-                    if (Conversions.Instance.TryParse(in inputValues[i], out uint index) && index >= _offset)
+                    if (Conversions.Instance.TryParse(in inputValues[i], out uint index))
+                    {
+                        if (index < _offset)
+                        {
+                            throw Contracts.Except("Encountered 0 index while parsing a 1-based dataset");
+                        }
                         editor.Values[i] = index - _offset + 1;
+                    }
                     else
-                        editor.Values[i] = _na;
+                        throw Contracts.Except($"Encountered non-parsable index '{inputValues[i]}' while parsing dataset");
                 }
                 output.FeatureKeys = editor.Commit();
             }
@@ -354,7 +357,7 @@ private sealed class OutputMapper
         {
             private readonly uint _keyMax;
             private readonly BufferBuilder<float> _bldr;
-            private readonly bool[] _indexUsed;
+            private readonly BitArray _indexUsed;
 
             public OutputMapper(int keyCount)
             {
@@ -363,7 +366,7 @@ public OutputMapper(int keyCount)
                 // incur any sort of implicit value conversions.
                 _keyMax = (uint)keyCount;
                 _bldr = new BufferBuilder<float>(FloatAdder.Instance);
-                _indexUsed = new bool[_keyMax];
+                _indexUsed = new BitArray((int)_keyMax);
             }
 
             public void Map(IntermediateOut intermediate, Output output)
@@ -383,14 +386,14 @@ private void MapCore(ref VBuffer<uint> keys, ref VBuffer<float> values, Output o
 
                 // The output vector could be sparse, so we use BufferBuilder here.
                 _bldr.Reset((int)_keyMax, false);
-                Array.Clear(_indexUsed, 0, _indexUsed.Length);
+                _indexUsed.SetAll(false);
                 for (int i = 0; i < keys.Length; ++i)
                 {
                     var key = keysValues[i];
                     if (key == 0 || key > _keyMax)
                         continue;
                     if (_indexUsed[(int)key - 1])
-                        continue;
+                        throw Contracts.Except("Duplicate keys found in dataset");
                     _bldr.AddFeature((int)key - 1, valuesValues[i]);
                     _indexUsed[(int)key - 1] = true;
                 }
@@ -411,9 +414,11 @@ private sealed class TextDataView : IDataView
             private readonly IHost _host;
             private readonly IMultiStreamSource _files;
 
-            public TextDataView(IHostEnvironment env, IMultiStreamSource files = null)
+            public TextDataView(IHostEnvironment env, IMultiStreamSource files)
             {
                 Contracts.CheckValue(env, nameof(env));
+                env.CheckValue(files, nameof(files));
+
                 _host = env.Register("TextDataView");
                 _files = files;
 
@@ -424,7 +429,7 @@ public TextDataView(IHostEnvironment env, IMultiStreamSource files = null)
 
             public long? GetRowCount()
             {
-                if (_files == null || _files.Count == 0)
+                if (_files.Count == 0)
                     return 0;
                 return null;
             }
@@ -461,7 +466,7 @@ public Cursor(TextDataView parent, bool isActive)
                 {
                     _parent = parent;
                     _isActive = isActive;
-                    if (_parent._files == null || _parent._files.Count == 0)
+                    if (_parent._files.Count == 0)
                     {
                         // Rather than corrupt MoveNextCore with a bunch of custom logic for
                         // the empty file case and make that less efficient, be slightly inefficient
@@ -513,9 +518,9 @@ public override bool IsColumnActive(DataViewSchema.Column column)
                 protected override bool MoveNextCore()
                 {
                     Ch.AssertValue(_currReader);
-                    Ch.Assert(-1 <= _fileIdx && _fileIdx < (_parent._files == null ? 0 : _parent._files.Count));
+                    Ch.Assert(-1 <= _fileIdx && _fileIdx < _parent._files.Count);
 
-                    var count = _parent._files == null ? 0 : _parent._files.Count;
+                    var count = _parent._files.Count;
                     for (; ; )
                     {
                         var line = _currReader.ReadLine();
@@ -570,7 +575,7 @@ internal SvmLightLoader(IHostEnvironment env, Options options = null, IMultiStre
                     _featureCount = (ulong)options.InputSize;
                 else
                 {
-                    if (dataSample == null)
+                    if (dataSample == null || dataSample.Count == 0)
                         throw env.Except("If the number of features is not specified, a dataset must be provided to infer it.");
                     var data = GetData(_host, options.NumberOfRows, dataSample);
                     _featureCount = InferMax(_host, data) + (ulong)(_indicesKind == FeatureIndices.ZeroBased ? 1 : 0);
@@ -580,7 +585,7 @@ internal SvmLightLoader(IHostEnvironment env, Options options = null, IMultiStre
             else
             {
                 // We need to train a ValueToKeyMappingTransformer.
-                if (dataSample == null)
+                if (dataSample == null || dataSample.Count == 0)
                     throw env.Except("To use the text feature names option, a dataset must be provided");
 
                 var data = GetData(_host, options.NumberOfRows, dataSample);
@@ -644,7 +649,7 @@ void ICanSaveModel.Save(ModelSaveContext ctx)
 
         private DataViewSchema CreateOutputSchema()
         {
-            var data = GetData(_host, null, null);
+            var data = GetData(_host, null, new MultiFileSource(null));
             var indexParser = new IndexParser(_indicesKind == FeatureIndices.ZeroBased, _featureCount);
             var schemaDef = SchemaDefinition.Create(typeof(Indices));
             schemaDef[nameof(Indices.FeatureKeys)].ColumnType = new KeyDataViewType(typeof(uint), _featureCount);
@@ -752,8 +757,6 @@ private static ITransformer CreateOutputTransformer(IHostEnvironment env, int ke
             {
                 outputTransformer = new CustomMappingTransformer<IntermediateOut, Output>(env,
                     outputMapper.Map, null, outputSchemaDefinition: schemaDef);
-                //outputTransformer = new CustomMappingTransformer<IntermediateOut, Output>(env,
-                //    outputMapper.Map, null, outputSchemaDefinition: schemaDef);
             }
 
             string[] toKeep = { "Label", "Weight", "GroupId", "Comment", "Features" };
@@ -764,6 +767,8 @@ private static ITransformer CreateOutputTransformer(IHostEnvironment env, int ke
 
         public IDataView Load(IMultiStreamSource input)
         {
+            _host.CheckValue(input, nameof(input));
+
             var data = GetData(_host, null, input);
             var indexParser = new IndexParser(_indicesKind == FeatureIndices.ZeroBased, _featureCount);
             var keyVectorsToIndexVectors = _keyVectorsToIndexVectors ??
diff --git a/src/Microsoft.ML.Transforms/SvmLight/SvmLightLoaderSaverCatalog.cs b/src/Microsoft.ML.Transforms/SvmLight/SvmLightLoaderSaverCatalog.cs
@@ -4,10 +4,9 @@
 
 using System.IO;
 using System.Linq;
-using Microsoft.ML.Data;
 using Microsoft.ML.Runtime;
 
-namespace Microsoft.ML.Transforms
+namespace Microsoft.ML.Data
 {
     public static class SvmLightLoaderSaverCatalog
     {
@@ -22,8 +21,8 @@ public static class SvmLightLoaderSaverCatalog
         /// it should be set to false.</param>
         /// <param name="dataSample">A data sample to be used for determining the number of features in the Features column.</param>
         public static SvmLightLoader CreateSvmLightLoader(this DataOperationsCatalog catalog,
-            int inputSize = 0,
             long? numberOfRows = null,
+            int inputSize = 0,
             bool zeroBased = false,
             IMultiStreamSource dataSample = null)
             => new SvmLightLoader(CatalogUtils.GetEnvironment(catalog), new SvmLightLoader.Options()
@@ -54,14 +53,14 @@ public static SvmLightLoader CreateSvmLightLoaderWithFeatureNames(this DataOpera
         /// <param name="numberOfRows">The number of rows from the sample to be used for determining the number of features.</param>
         public static IDataView LoadFromSvmLightFile(this DataOperationsCatalog catalog,
             string path,
+            long? numberOfRows = null,
             int inputSize = 0,
-            bool zeroBased = false,
-            long? numberOfRows = null)
+            bool zeroBased = false)
         {
             Contracts.CheckNonEmpty(path, nameof(path));
 
             var file = new MultiFileSource(path);
-            var loader = catalog.CreateSvmLightLoader(inputSize, numberOfRows, zeroBased, file);
+            var loader = catalog.CreateSvmLightLoader(numberOfRows, inputSize, zeroBased, file);
             return loader.Load(file);
         }
 
diff --git a/src/Microsoft.ML.Transforms/SvmLight/SvmLightSaver.cs b/src/Microsoft.ML.Transforms/SvmLight/SvmLightSaver.cs
@@ -10,12 +10,11 @@
 using Microsoft.ML.Data;
 using Microsoft.ML.Data.IO;
 using Microsoft.ML.Runtime;
-using Microsoft.ML.Transforms;
 
 [assembly: LoadableClass(SvmLightSaver.Summary, typeof(SvmLightSaver), typeof(SvmLightSaver.Arguments), typeof(SignatureDataSaver),
     "SVM-Light Saver", SvmLightSaver.LoadName, "SvmLight", "Svm")]
 
-namespace Microsoft.ML.Transforms
+namespace Microsoft.ML.Data
 {
     /// <summary>
     /// The SVM-light saver is a saver class that is capable of saving the label,
diff --git a/test/Microsoft.ML.TestFramework/TestCommandBase.cs b/test/Microsoft.ML.TestFramework/TestCommandBase.cs
@@ -2199,7 +2199,7 @@ public void CommandShowDataSvmLight()
             path = CreateOutputPath("DataC.txt");
             File.WriteAllLines(path.Path, new string[] {
                 "-1 aurora:1 chagrin:2",
-                "1 chagrin:3 euclid:4 chagrin:5"
+                "1 chagrin:3 euclid:4"
             });
             TestInCore("showdata", path.Path, modelPath, "");
 
diff --git a/test/Microsoft.ML.Tests/SvmLightTests.cs b/test/Microsoft.ML.Tests/SvmLightTests.cs
