dotnet / dotnet/machinelearning

Incorrect code in BinaryModelParameters classes

Open
#4,381 4 comments 1 reaction 2 assignees Claimed by @rosebyte View on GitHub
area-Trees enhancement in-pr Priority:3
Dominant language
C#
Stars
9.4k
Forks
2k
Avg merge
2d 20h
Merged PRs (30d)
11

Description

There is a problem with the code inside the Create method of the following classes:
1. [LinearBinaryModelParameters](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.StandardTrainers/Standard/LinearModelParameters.cs#L473)
2. [GamBinaryModelParameters](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.FastTree/GamClassification.cs#L233)
3. [FastTreeBinaryModelParameters](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.FastTree/FastTreeClassification.cs#L87)
4. [FastForestBinaryModelParameters](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.FastTree/RandomForestClassification.cs#L102)

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](https://github.com/dotnet/machinelearning/pull/4306#discussion_r336265838) (under question number 2) while working on my PR #4306 . There, @yaeldekel [responded](https://github.com/dotnet/machinelearning/pull/4306#discussion_r337981820) 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](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.FastTree/RandomForestClassification.cs#L110) '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](https://github.com/dotnet/machinelearning/blob/d531ea801a34a3018b8fa2a2f352902eb703cd25/src/Microsoft.ML.StandardTrainers/Standard/LinearModelParameters.cs#L484), 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**.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.