[WIP] Fix suspicious fragments found by PVS-Studio analyzer

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

@@ -36,7 +36,7 @@ private EstimatorChain(IHostEnvironment env, IEstimator<ITransformer>[] estimato
             _host = env?.Register(nameof(EstimatorChain<TLastTransformer>));
             _estimators = estimators ?? new IEstimator<ITransformer>[0];
             _scopes = scopes ?? new TransformerScope[0];
-            LastEstimator = estimators.LastOrDefault() as IEstimator<TLastTransformer>;
+            LastEstimator = _estimators.LastOrDefault() as IEstimator<TLastTransformer>;
             _needCacheAfter = needCacheAfter ?? new bool[0];
 
             Contracts.Assert((_host != null) == _needCacheAfter.Any(x => x));

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

@@ -88,7 +88,7 @@ public TransformerChain(IEnumerable<ITransformer> transformers, IEnumerable<Tran
 
             _transformers = transformers?.ToArray() ?? new ITransformer[0];
             _scopes = scopes?.ToArray() ?? new TransformerScope[0];
-            LastTransformer = transformers.LastOrDefault() as TLastTransformer;
+            LastTransformer = _transformers.LastOrDefault() as TLastTransformer;
 
             Contracts.Check((_transformers.Length > 0) == (LastTransformer != null));
             Contracts.Check(_transformers.Length == _scopes.Length);

src/Microsoft.ML.Ensemble/PipelineEnsemble.cs

@@ -667,7 +667,7 @@ private static int CheckKeyLabelColumnCore<T>(IHostEnvironment env, PredictorMod
                     throw env.Except("Label column of model {0} has different type than model 0", i);
 
                 var mdType = labelCol.Annotations.Schema.GetColumnOrNull(AnnotationUtils.Kinds.KeyValues)?.Type;
-                if (!mdType.Equals(keyValuesType))
+                if (!keyValuesType.Equals(mdType))
                     throw env.Except("Label column of model {0} has different key value type than model 0", i);
                 labelCol.GetKeyValues(ref curLabelNames);
                 if (!AreEqual(in labelNames, in curLabelNames))

src/Microsoft.ML.FastTree/BoostingFastTree.cs

@@ -41,9 +41,6 @@ private protected override void CheckOptions(IChannel ch)
             if (FastTreeTrainerOptions.CompressEnsemble && FastTreeTrainerOptions.WriteLastEnsemble)
                 throw ch.Except("Ensemble compression cannot be done when forcing to write last ensemble (hl)");
 
-            if (FastTreeTrainerOptions.NumberOfLeaves > 2 && FastTreeTrainerOptions.HistogramPoolSize > FastTreeTrainerOptions.NumberOfLeaves - 1)
-                throw ch.Except("Histogram pool size (ps) must be at least 2.");
-
             if (FastTreeTrainerOptions.NumberOfLeaves > 2 && FastTreeTrainerOptions.HistogramPoolSize > FastTreeTrainerOptions.NumberOfLeaves - 1)
                 throw ch.Except("Histogram pool size (ps) must be at most numLeaves - 1.");
 

src/Microsoft.ML.Transforms/Text/LdaSingleBox.cs

@@ -139,7 +139,7 @@ public void AllocateModelMemory(int numTopic, int numVocab, long tableSize, long
             Contracts.Check(numVocab >= 0);
             Contracts.Check(tableSize >= 0);
             Contracts.Check(aliasTableSize >= 0);
-            LdaInterface.AllocateModelMemory(_engine, numVocab, numTopic, tableSize, aliasTableSize);
+            LdaInterface.AllocateModelMemory(_engine, numTopic, numVocab, tableSize, aliasTableSize);
         }
 
         public void AllocateDataMemory(int docNum, long corpusSize)

src/Microsoft.ML.Transforms/Text/LdaTransform.cs

@@ -300,7 +300,7 @@ internal LdaState(IExceptionContext ectx, ModelLoadContext ctx)
                     false,
                     InfoEx.MaximumTokenCountPerDocument);
 
-                _ldaTrainer.AllocateModelMemory(_numVocab, InfoEx.NumberOfTopics, memBlockSize, aliasMemBlockSize);
+                _ldaTrainer.AllocateModelMemory(InfoEx.NumberOfTopics, _numVocab, memBlockSize, aliasMemBlockSize);
 
                 for (int i = 0; i < _numVocab; i++)
                 {

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

@@ -6,6 +6,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Reflection;
 using System.Text.RegularExpressions;
 using Microsoft.ML.Calibrators;
 using Microsoft.ML.Core.Tests.UnitTests;
@@ -1537,6 +1538,45 @@ public void EntryPointCalibrate()
             Done();
         }
 
+        [Theory]
+        [InlineData(0)]
+        [InlineData(1)]
+        [InlineData(2)]
+        public void PipelineEnsembleValidatesKeyValuesMetadata(int metadataSize)
+        {
+            PredictorModel CreateModel(int keyValuesCount)
+            {
+                var annotations = new DataViewSchema.Annotations.Builder();
+                if (keyValuesCount > 0)
+                {
+                    var values = new VBuffer<int>(keyValuesCount, Enumerable.Range(0, keyValuesCount).ToArray());
+                    annotations.AddKeyValues(keyValuesCount, NumberDataViewType.Int32,
+                        (ref VBuffer<int> destination) => values.CopyTo(ref destination));
+                }
+
+                var schema = new DataViewSchema.Builder();
+                schema.AddColumn("Label", new KeyDataViewType(typeof(uint), 2), annotations.ToAnnotations());
+                var data = new EmptyDataView(Env, schema.ToSchema());
+                return new PredictorModelImpl(Env, new RoleMappedData(data, label: "Label", feature: null),
+                    data, new PriorModelParameters(Env, 0.5f));
+            }
+
+            var models = new[] { CreateModel(2), CreateModel(metadataSize) };
+            if (metadataSize == 2)
+            {
+                Assert.NotNull(SchemaBindablePipelineEnsembleBase.Create(Env, models, new Average(Env),
+                    AnnotationUtils.Const.ScoreColumnKind.BinaryClassification));
+            }
+            else
+            {
+                var exception = Assert.Throws<TargetInvocationException>(() =>
+                    SchemaBindablePipelineEnsembleBase.Create(Env, models, new Average(Env),
+                        AnnotationUtils.Const.ScoreColumnKind.BinaryClassification));
+                var innerException = Assert.IsType<InvalidOperationException>(exception.InnerException);
+                Assert.Contains("Label column of model 1 has different key value type than model 0", innerException.Message);
+            }
+        }
+
         [Fact]
         public void EntryPointPipelineEnsemble()
         {

test/Microsoft.ML.Tests/ChainTests.cs

@@ -0,0 +1,94 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Collections.Generic;
+using System.Reflection;
+using Microsoft.ML.Data;
+using Microsoft.ML.RunTests;
+using Microsoft.ML.Runtime;
+using Microsoft.ML.Transforms;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Microsoft.ML.Tests
+{
+    public class ChainTests : TestDataPipeBase
+    {
+        public ChainTests(ITestOutputHelper helper) : base(helper)
+        {
+        }
+
+        private class MyData
+        {
+            public float Feature { get; set; }
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void EstimatorChainAcceptsNullOrEmptyEstimators(bool useNull)
+        {
+            var constructor = typeof(EstimatorChain<ITransformer>).GetConstructor(
+                BindingFlags.Instance | BindingFlags.NonPublic, null,
+                new[] { typeof(IHostEnvironment), typeof(IEstimator<ITransformer>[]), typeof(TransformerScope[]), typeof(bool[]) }, null);
+            Assert.NotNull(constructor);
+
+            var chain = (EstimatorChain<ITransformer>)constructor.Invoke(new object[]
+            {
+                null,
+                useNull ? null : Array.Empty<IEstimator<ITransformer>>(),
+                useNull ? null : Array.Empty<TransformerScope>(),
+                useNull ? null : Array.Empty<bool>()
+            });
+
+            Assert.Null(chain.LastEstimator);
+            var data = ML.Data.LoadFromEnumerable(new[] { new MyData() });
+            Assert.Same(data, chain.Fit(data).Transform(data));
+
+            var estimator = ML.Transforms.CopyColumns("F1", "Feature");
+            Assert.Same(estimator, chain.Append(estimator).LastEstimator);
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void TransformerChainAcceptsNullOrEmptyTransformers(bool useNull)
+        {
+            var chain = new TransformerChain<ITransformer>(
+                useNull ? null : Array.Empty<ITransformer>(),
+                useNull ? null : Array.Empty<TransformerScope>());
+
+            Assert.Null(chain.LastTransformer);
+            Assert.Empty(chain);
+            var data = ML.Data.LoadFromEnumerable(new[] { new MyData() });
+            Assert.Same(data, chain.Transform(data));
+            Assert.Same(data.Schema, chain.GetOutputSchema(data.Schema));
+        }
+
+        [Fact]
+        public void TransformerChainEnumeratesTransformersOnce()
+        {
+            var data = ML.Data.LoadFromEnumerable(new[] { new MyData() });
+            var first = ML.Transforms.CopyColumns("F1", "Feature").Fit(data);
+            var last = ML.Transforms.CopyColumns("F2", "F1").Fit(first.Transform(data));
+            int enumerationCount = 0;
+
+            IEnumerable<ITransformer> GetTransformers()
+            {
+                enumerationCount++;
+                yield return first;
+                yield return last;
+            }
+
+            var chain = new TransformerChain<ColumnCopyingTransformer>(
+                GetTransformers(), new[] { TransformerScope.Everything, TransformerScope.Everything });
+
+            Assert.Equal(1, enumerationCount);
+            Assert.Equal(new ITransformer[] { first, last }, chain);
+            Assert.Same(last, chain.LastTransformer);
+            Assert.Equal(typeof(float), chain.Transform(data).Schema["F2"].Type.RawType);
+        }
+    }
+}

test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs

@@ -54,6 +54,36 @@ public void FastTreeBinaryEstimator()
             Done();
         }
 
+        [Theory]
+        [InlineData(5, -1)]
+        [InlineData(5, 0)]
+        [InlineData(5, 1)]
+        [InlineData(5, 2)]
+        [InlineData(5, 4)]
+        [InlineData(5, 5)]
+        [InlineData(2, 1)]
+        public void FastTreeHistogramPoolSizeValidation(int numberOfLeaves, int histogramPoolSize)
+        {
+            var data = ML.Data.LoadFromEnumerable(
+                SamplesUtils.DatasetUtils.GenerateBinaryLabelFloatFeatureVectorFloatWeightSamples(100).ToList());
+            var trainer = ML.BinaryClassification.Trainers.FastTree(new FastTreeBinaryTrainer.Options
+            {
+                NumberOfThreads = 1,
+                NumberOfTrees = 1,
+                NumberOfLeaves = numberOfLeaves,
+                HistogramPoolSize = histogramPoolSize,
+                MinimumExampleCountPerLeaf = 1,
+            });
+
+            if (numberOfLeaves > 2 && histogramPoolSize > numberOfLeaves - 1)
+            {
+                var exception = Assert.Throws<InvalidOperationException>(() => trainer.Fit(data));
+                Assert.Contains("Histogram pool size (ps) must be at most numLeaves - 1.", exception.Message);
+            }
+            else
+                Assert.NotNull(trainer.Fit(data));
+        }
+
         [LightGBMFact]
         public void LightGBMBinaryEstimator()
         {