Fixes #3878. About calling Fit more than once on Multiclass LightGBM trainer.

[antoniovs1029]

Fixes #3878, by initializing the values of _tlcNumClass and _numClass everytime Fit() is called on a Multiclass LightGBM trainer. This is done under the assumption that trainers should behave as if they were stateless, and every call to Fit() should re-initialize those values.

As discussed offline with @yaeldekel , _tlcNumClass and _numClass hold the same value if there were no NaN labels on the dataset used for training. If there were NaN labels then _tlcNumClass = _numClass - 1. Mantaining both fields is necessary, because here NaN labels are replaced to be a new class, and then, here, when training the WrappedLightGbmTraining it is as if NaN labels were an extra class, but then here, when creating the Predictors, only _tlcNumClass predictors are created. So, for example, if I have a dataset with 3 classes (0-2), but some rows have NaN values on their labels, then NaN values get converted to "3", _numClass is equal to "4" and WrappedLightGbmTraining trains as if there were truly 4 classes... but _tlcNumClass is equal to "3" and when creating the Predictors, only 3 predictors are created (one for each of the original classes ignoring the "fake NaN class").

The above wasn't documented in the code, but after doing some tests it seems that is how it is supposed to behave, and so I added some comments explaining this, as @yaeldekel and I agree that the above isn't really clear directly from reading the code, and the names _tlcNumClass and _numClass are somewhat obscure.

I also added a test.

[codecov]

Codecov Report

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

@@            Coverage Diff            @@
##             master    #4608   +/-   ##
=========================================
  Coverage          ?   75.64%           
=========================================
  Files             ?      938           
  Lines             ?   168669           
  Branches          ?    18210           
=========================================
  Hits              ?   127584           
  Misses            ?    36056           
  Partials          ?     5029

Flag	Coverage Δ
#Debug	75.64% <84.13%> (?)
#production	71.25% <84.13%> (?)
#test	90.45% <ø> (?)

Impacted Files	Coverage Δ
...osoft.ML.Transforms/Text/TokenizingByCharacters.cs	95.32% <ø> (ø)
...ML.Data/Transforms/ColumnConcatenatingEstimator.cs	83.58% <ø> (ø)
src/Microsoft.ML.TimeSeries/IidSpikeDetector.cs	78.43% <ø> (ø)
src/Microsoft.ML.FastTree/FastTreeRanking.cs	48.68% <ø> (ø)
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	85.19% <ø> (ø)
...c/Microsoft.ML.Transforms/MissingValueReplacing.cs	76.77% <ø> (ø)
...oft.ML.StandardTrainers/Standard/SdcaRegression.cs	95.83% <ø> (ø)
...rc/Microsoft.ML.FastTree/FastTreeClassification.cs	81.99% <ø> (ø)
src/Microsoft.ML.ImageAnalytics/ImageResizer.cs	85.01% <ø> (ø)
...soft.ML.Transforms/Text/NgramHashingTransformer.cs	88.79% <ø> (ø)
... and 112 more

[yaeldekel]

    private protected IParallel ParallelTraining;

This should be readonly. #Resolved

---

Refers to: src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:291 in 1aaa77f. [](commit_id = 1aaa77f, deletion_comment = False)

[yaeldekel]

    private protected Dictionary<string, object> GbmOptions;

This is one of the trainer fields that are not readonly, but I see no reason why it shouldn't be readonly. I was actually wondering whether we need this field at all - it is used to create a string of options passed to the LightGbm trainer, perhaps this string can be created using the LightGbmTrainerOptions field and output of other methods called in LightGbmTrainerBase.TrainModelCore? #Resolved

---

Refers to: src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:289 in 1aaa77f. [](commit_id = 1aaa77f, deletion_comment = False)

[yaeldekel]

            var gbmNative = WrappedLightGbmTraining.Train(ch, pch, gbmParams, gbmDataSet, numIteration: numberOfTrainingIterations);

Unrelated: this is IDisposable, should be using .... #Resolved

---

Refers to: test/Microsoft.ML.Tests/TrainerEstimators/TreeEstimators.cs:495 in 1aaa77f. [](commit_id = 1aaa77f, deletion_comment = False)

[antoniovs1029]

    private protected IParallel ParallelTraining;

It seems to me the only reason ParallelTraining and GbmOptions were not readonly, was because they weren't initialized in the constructor, but in a helper method called by the constructor (i.e. InitParallelTraining). So in order to make them readonly, I also had to move their initialization to the constructor of LightGbmTrainerBase.

---

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

---

Refers to: src/Microsoft.ML.LightGbm/LightGbmTrainerBase.cs:291 in 1aaa77f. [](commit_id = 1aaa77f, deletion_comment = False)

[yaeldekel]

I think these comments can be safely deleted. #Resolved

[yaeldekel]