Incorrect code in BinaryModelParameters classes

[antoniovs1029]

There is a problem with the code inside the Create method of the following classes:

1. LinearBinaryModelParameters
2. GamBinaryModelParameters
3. FastTreeBinaryModelParameters
4. FastForestBinaryModelParameters

The problem is that in those 4 cases, each Create method has a return statement that returns a SchemaBindableCalibratedModelParameters<,> object. Notice that these Create methods are supposed to load from disk, in each case, a BinaryModelParameters object of the appropriate type. So this doesn't make sense, since the SBCMP class is not supposed to be used as a BinaryModelParameter, and thus, these classes shouldn't be loaded as SBCMP objects.

I pointed at this problem inside this comment (under question number 2) while working on my PR #4306 . There, @yaeldekel responded that these pieces of code seem 'very wrong' to her, and that they might be the result of some legacy code that is no longer valid. Specifically she mentioned that in the past Calibrators where a field of predictors, and predictors were responsible of loading them at deserialization time. This is no longer the case, and her guess is that the code that I've pointed to is no longer valid.

Even more, she believes that the return new SBCMP statements I've mentioned are actually unreachable now, since they all appear in branches that only execute when there is no calibrator to load inside the BinaryModelParameters (e.g. this 'if statement'). Since predictors are no longer in charge of loading calibrators, then those paths are unreachable, and then the Create methods I've mentioned, always return the predictor of the appropriate type anyway, without getting into creating SBCMP objects.

In the case of the LinearBinaryModelParameters Create method, a ParameterMixingCalibratedModelParameters<,> object could also be returned, but this also seems invalid and unreachable for the same reasons already described for SBCMP.

Perhaps further investigation is needed to clarify all of this, and if those pieces of code are truly unreachable and no longer valid, then it might be better to remove them.

[antoniovs1029]

Notice that the 4 create methods I've pointed at in this issue are also affected by issue #4385 . In despite of this, these two issues are independent, and both require separate solutions.

The other issue refers to the fact that these create methods are not being called, but they should be called when loading models from disk (whenever the model uses any of the 4 classes I've mentioned). In contrast, this issue refers to the problem that the content of these create methods seems wrong, and might need to change. Also, the other issue affects other classes as well, whereas this issue is particular of the 4 classes I already mentioned.

[antoniovs1029]

The LightGbmBinaryModelParameters class also presents the same problem as the other classes.

[antoniovs1029]

After #4485 solved #4385, these create methods I've mentioned in this issue are now getting called, making it easier to debug them.

As an experiment, I added code to throw exceptions in the code that we suspect is unreachable, and then I ran all the existing tests. As expected, no exception was thrown, suggesting that these pieces of code are actually unreachable (at least in the existing tests).

So I guess the way to go is to actually remove these pieces of code?