Address some comments

Author: wschin

src/Microsoft.ML.FastTree/FastTreeArguments.cs

@@ -75,38 +75,43 @@ public sealed class Options : BoostedTreeOptions, IFastTreeTrainerFactory
         {
             [Argument(ArgumentType.LastOccurenceWins, HelpText = "Comma seperated list of gains associated to each relevance label.", ShortName = "gains")]
             [TGUI(NoSweep = true)]
-            public string CustomGains = "0,3,7,15,31";
+            public double[] CustomGains = new double[] { 0, 3, 7, 15, 31 };
 
             [Argument(ArgumentType.LastOccurenceWins, HelpText = "Train DCG instead of NDCG", ShortName = "dcg")]
-            public bool TrainDcg;
+            public bool UseDcg;
 
             // REVIEW: Hiding sorting for now. Should be an enum or component factory.
+            [BestFriend]
             [Argument(ArgumentType.LastOccurenceWins,
                 HelpText = "The sorting algorithm to use for DCG and LambdaMart calculations [DescendingStablePessimistic/DescendingStable/DescendingReverse/DescendingDotNet]",
                 ShortName = "sort",
                 Hide = true)]
             [TGUI(NotGui = true)]
-            public string SortingAlgorithm = "DescendingStablePessimistic";
+            internal string SortingAlgorithm = "DescendingStablePessimistic";
 
             [Argument(ArgumentType.AtMostOnce, HelpText = "max-NDCG truncation to use in the Lambda Mart algorithm", ShortName = "n", Hide = true)]
             [TGUI(NotGui = true)]
-            public int LambdaMartMaxTruncation = 100;
+            public int NdcgTruncationLevel = 100;
 
+            [BestFriend]
             [Argument(ArgumentType.LastOccurenceWins, HelpText = "Use shifted NDCG", Hide = true)]
             [TGUI(NotGui = true)]
-            public bool ShiftedNdcg;
+            internal bool ShiftedNdcg;
 
+            [BestFriend]
             [Argument(ArgumentType.AtMostOnce, HelpText = "Cost function parameter (w/c)", ShortName = "cf", Hide = true)]
             [TGUI(NotGui = true)]
-            public char CostFunctionParam = 'w';
+            internal char CostFunctionParam = 'w';
 
+            [BestFriend]
             [Argument(ArgumentType.LastOccurenceWins, HelpText = "Distance weight 2 adjustment to cost", ShortName = "dw", Hide = true)]
             [TGUI(NotGui = true)]
-            public bool DistanceWeight2;
+            internal bool DistanceWeight2;
 
+            [BestFriend]
             [Argument(ArgumentType.LastOccurenceWins, HelpText = "Normalize query lambdas", ShortName = "nql", Hide = true)]
             [TGUI(NotGui = true)]
-            public bool NormalizeQueryLambdas;
+            internal bool NormalizeQueryLambdas;
 
             public Options()
             {
@@ -129,7 +134,7 @@ internal override void Check(IExceptionContext ectx)
 #if OLD_DATALOAD
                 ectx.CheckUserArg(0 <= secondaryMetricShare && secondaryMetricShare <= 1, "secondaryMetricShare", "secondaryMetricShare must be between 0 and 1.");
 #endif
-                ectx.CheckUserArg(0 < LambdaMartMaxTruncation, nameof(LambdaMartMaxTruncation), "lambdaMartMaxTruncation must be positive.");
+                ectx.CheckUserArg(0 < NdcgTruncationLevel, nameof(NdcgTruncationLevel), "lambdaMartMaxTruncation must be positive.");
             }
         }
     }
@@ -387,14 +392,6 @@ public abstract class TreeOptions : TrainerInputBaseWithGroupId
         [TGUI(NotGui = true)]
         public bool CompressEnsemble;
 
-        /// <summary>
-        /// Maximum Number of trees after compression.
-        /// </summary>
-        // REVIEW: Not used.
-        [Argument(ArgumentType.AtMostOnce, HelpText = "Maximum Number of trees after compression", ShortName = "cmpmax", Hide = true)]
-        [TGUI(NotGui = true)]
-        public int MaximumTreeCountAfterCompression = -1;
-
         /// <summary>
         /// Print metrics graph for the first test set.
         /// </summary>

src/Microsoft.ML.FastTree/FastTreeRanking.cs

@@ -131,7 +131,7 @@ private Double[] GetLabelGains()
             try
             {
                 Host.AssertValue(FastTreeTrainerOptions.CustomGains);
-                return FastTreeTrainerOptions.CustomGains.Split(',').Select(k => Convert.ToDouble(k.Trim())).ToArray();
+                return FastTreeTrainerOptions.CustomGains;
             }
             catch (Exception ex)
             {
@@ -143,26 +143,17 @@ private Double[] GetLabelGains()
 
         private protected override void CheckOptions(IChannel ch)
         {
-            if (!string.IsNullOrEmpty(FastTreeTrainerOptions.CustomGains))
+            if (FastTreeTrainerOptions.CustomGains != null)
             {
-                var stringGain = FastTreeTrainerOptions.CustomGains.Split(',');
-                if (stringGain.Length < 5)
+                var gains = FastTreeTrainerOptions.CustomGains;
+                if (gains.Length < 5)
                 {
                     throw ch.ExceptUserArg(nameof(FastTreeTrainerOptions.CustomGains),
-                        "{0} an invalid number of gain levels. We require at least 5. Make certain they're comma separated.",
-                        stringGain.Length);
+                        "Has {0} gain levels. We require at least 5 elements.",
+                        gains.Length);
                 }
-                Double[] gain = new Double[stringGain.Length];
-                for (int i = 0; i < stringGain.Length; ++i)
-                {
-                    if (!Double.TryParse(stringGain[i], out gain[i]))
-                    {
-                        throw ch.ExceptUserArg(nameof(FastTreeTrainerOptions.CustomGains),
-                            "Could not parse '{0}' as a floating point number", stringGain[0]);
-                    }
-                }
-                DcgCalculator.LabelGainMap = gain;
-                Dataset.DatasetSkeleton.LabelGainMap = gain;
+                DcgCalculator.LabelGainMap = gains;
+                Dataset.DatasetSkeleton.LabelGainMap = gains;
             }
 
             ch.CheckUserArg((FastTreeTrainerOptions.EarlyStoppingRule == null && !FastTreeTrainerOptions.EnablePruning) || (FastTreeTrainerOptions.EarlyStoppingMetrics == 1 || FastTreeTrainerOptions.EarlyStoppingMetrics == 3), nameof(FastTreeTrainerOptions.EarlyStoppingMetrics),
@@ -498,7 +489,7 @@ private enum DupeIdInfo
 
             // parameters
             private int _maxDcgTruncationLevel;
-            private bool _trainDcg;
+            private bool _useDcg;
             // A lookup table for the sigmoid used in the lambda calculation
             // Note: Is built for a specific sigmoid parameter, so assumes this will be constant throughout computation
             private double[] _sigmoidTable;
@@ -570,9 +561,9 @@ public LambdaRankObjectiveFunction(Dataset trainset, short[] labels, Options opt
                     _labelCounts[q] = new int[relevancyLevel];
 
                 // precomputed arrays
-                _maxDcgTruncationLevel = options.LambdaMartMaxTruncation;
-                _trainDcg = options.TrainDcg;
-                if (_trainDcg)
+                _maxDcgTruncationLevel = options.NdcgTruncationLevel;
+                _useDcg = options.UseDcg;
+                if (_useDcg)
                 {
                     _inverseMaxDcgt = new double[Dataset.NumQueries];
                     for (int q = 0; q < Dataset.NumQueries; ++q)
@@ -875,7 +866,7 @@ protected override void GetGradientInOneQuery(int query, int threadIndex)
 
                         // Continous cost function and shifted NDCG require a re-sort and recomputation of maxDCG
                         // (Change of scores in the former and scores and labels in the latter)
-                        if (!_trainDcg && (_costFunctionParam == 'c' || _useShiftedNdcg))
+                        if (!_useDcg && (_costFunctionParam == 'c' || _useShiftedNdcg))
                         {
                             PermutationSort(permutation, scoresToUse, labels, numDocuments, begin);
                             inverseMaxDcg = 1.0 / DcgCalculator.MaxDcgQuery(labels, begin, numDocuments, numDocuments, _labelCounts[query]);

src/Microsoft.ML.FastTree/RegressionTree.cs

@@ -21,11 +21,11 @@ public abstract class RegressionTreeBase
         private readonly InternalRegressionTree _tree;
 
         /// <summary>
-        /// See <see cref="LessThanOrEqualToThresholdChildren"/>.
+        /// See <see cref="LeftChild"/>.
         /// </summary>
         private readonly ImmutableArray<int> _lteChild;
         /// <summary>
-        /// See <see cref="GreaterThanThresholdChildren"/>.
+        /// See <see cref="RightChild"/>.
         /// </summary>
         private readonly ImmutableArray<int> _gtChild;
         /// <summary>
@@ -50,9 +50,9 @@ public abstract class RegressionTreeBase
         private readonly ImmutableArray<double> _splitGains;
 
         /// <summary>
-        /// <see cref="LessThanOrEqualToThresholdChildren"/>[i] is the i-th node's child index used when
-        /// (1) the numerical feature indexed by <see cref="NumericalSplitFeatureIndexes"/>[i] is less than the
-        /// threshold <see cref="NumericalSplitThresholds"/>[i], or
+        /// <see cref="LeftChild"/>[i] is the i-th node's child index used when
+        /// (1) the numerical feature indexed by <see cref="NumericalSplitFeatureIndexes"/>[i] is less than or equal
+        /// to the threshold <see cref="NumericalSplitThresholds"/>[i], or
         /// (2) the categorical features indexed by <see cref="GetCategoricalCategoricalSplitFeatureRangeAt(int)"/>'s
         /// returned value with nodeIndex=i is NOT a sub-set of <see cref="GetCategoricalSplitFeaturesAt(int)"/> with
         /// nodeIndex=i.
@@ -63,14 +63,14 @@ public abstract class RegressionTreeBase
         /// bitwise complement operator in C#; for details, see
         /// https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/bitwise-complement-operator.
         /// </summary>
-        public IReadOnlyList<int> LessThanOrEqualToThresholdChildren => _lteChild;
+        public IReadOnlyList<int> LeftChild => _lteChild;
 
         /// <summary>
-        /// <see cref="GreaterThanThresholdChildren"/>[i] is the i-th node's child index used when the two conditions, (1) and (2),
-        /// described in <see cref="LessThanOrEqualToThresholdChildren"/>'s document are not true. Its return value follows the format
-        /// used in <see cref="LessThanOrEqualToThresholdChildren"/>.
+        /// <see cref="RightChild"/>[i] is the i-th node's child index used when the two conditions, (1) and (2),
+        /// described in <see cref="LeftChild"/>'s document are not true. Its return value follows the format
+        /// used in <see cref="LeftChild"/>.
         /// </summary>
-        public IReadOnlyList<int> GreaterThanThresholdChildren => _gtChild;
+        public IReadOnlyList<int> RightChild => _gtChild;
 
         /// <summary>
         /// <see cref="NumericalSplitFeatureIndexes"/>[i] is the feature index used the splitting function of the
@@ -99,7 +99,7 @@ public abstract class RegressionTreeBase
         /// <summary>
         /// Return categorical thresholds used at node indexed by nodeIndex. If the considered input feature does NOT
         /// matche any of values returned by <see cref="GetCategoricalSplitFeaturesAt(int)"/>, we call it a
-        /// less-than-threshold event and therefore <see cref="LessThanOrEqualToThresholdChildren"/>[nodeIndex] is the child node that input
+        /// less-than-threshold event and therefore <see cref="LeftChild"/>[nodeIndex] is the child node that input
         /// should go next. The returned value is valid only if <see cref="CategoricalSplitFlags"/>[nodeIndex] is true.
         /// </summary>
         public IReadOnlyList<int> GetCategoricalSplitFeaturesAt(int nodeIndex)