Fixes #4292 about using PFI with BPT and CMPB

[antoniovs1029]

Fixes #4292 about using PFI with a BinaryPredictionTransformer<> and classes derived from CalibratedModelParametersBase<>.

Approach of the solution

Regarding the 3 problems listed in #4292 the solutions were the following:

1. To call the correct 'create' method: as discussed here, I removed the constructor with overload (IHostEnvironment env, ModelLoadContext ctx) from the PMCMP<,> class, since its not needed after the changes on this PR. Without that constructor, the 'create' method gets called directly by CreateInstanceCore without changing anything else.
2. To create a PMCMP object with the correct parameter types: I used a similar approach as the one explained in my previous pull request Addresses #3976 about using PFI with a model loaded from disk #4262. That is, the calibrator and submodel of the PMCMP<> are loaded first, and then they are used to create a generic type at runtime for PMCMP<> but now using the correct parameter types.
3. To create a BPT<CMPB<>> object whereas a PMCMP<> was loaded: After trying different approaches that didn't work, and following the suggestions of @eerhardt the best way to fix this was to create a new attribute called PredictionTransformerLoadTypeAttribute, that would help the prediction transformer to know what's the correct type to use as a TModel when loading a model from disk. This attribute is applied to the PMCMP<> class, and it's used in PredictionTransformer.cs to decide the correct type for the BPT.

Also, after discussing it with @eerhardt , I also fixed these 3 problems for the classes that inherit from CalibratedModelParametersBase<,>; those are: ValueMapperCalibratedModelParameters<,>, and FeatureWeightsCalibratedModelParameters<,>. Notice that SchemaBindableCalibratedModelParameters<,> also inherits from CMPB<,>, but, as discussed here, it was decided not to fix it.

Changes implemented

• Changes in PredictionTransformer.cs and Calibrator.cs to implement the approach previously described
• Added a working sample for using PFI with BPT and CMPB while loading a model from disk. This is based entirely in the original sample.
• Added file CalibratedModelParametersTests.cs with tests that the CMPs modified in this PR are now being correctly loaded from disk.
• Changed a couple of tests in LbfgsTests.cs that failed because they used casts that now return 'null'. Notice that those casts involved using classes such as IPredictorProducing and PMCMP which are internal classes, so a regular user wouldn't have been able to use those casts. They have now been replaced with casts to the public classes BPT<CMPB<>> and CMPB<> using the correct type parameters of the model.

[codecov]

Codecov Report

Merging #4306 into master will increase coverage by 4.66%.
The diff coverage is 98.9%.

@@            Coverage Diff             @@
##           master    #4306      +/-   ##
==========================================
+ Coverage   70.08%   74.75%   +4.66%     
==========================================
  Files         706      907     +201     
  Lines      122440   159437   +36997     
  Branches    15235    17153    +1918     
==========================================
+ Hits        85809   119181   +33372     
- Misses      32130    35450    +3320     
- Partials     4501     4806     +305

Flag	Coverage Δ
#Debug	74.75% <98.9%> (+4.66%)	⬆️
#production	70.1% <97.05%> (+0.02%)	⬆️
#test	90.14% <100%> (?)

Impacted Files	Coverage Δ
...soft.ML.Tests/PermutationFeatureImportanceTests.cs	100% <100%> (ø)
src/Microsoft.ML.Data/Prediction/Calibrator.cs	78.68% <100%> (+3.02%)	⬆️
...crosoft.ML.Tests/CalibratedModelParametersTests.cs	100% <100%> (ø)
...Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs	98.02% <100%> (ø)
...Microsoft.ML.Data/Scorers/PredictionTransformer.cs	90.88% <94.59%> (-0.2%)	⬇️
...c/Microsoft.ML.FastTree/Utils/ThreadTaskManager.cs	79.48% <0%> (-20.52%)	⬇️
src/Microsoft.ML.AutoML/Sweepers/ISweeper.cs	59.49% <0%> (-7.6%)	⬇️
...L.AutoML/TrainerExtensions/TrainerExtensionUtil.cs	85.15% <0%> (-1.75%)	⬇️
...ML.Transforms/Text/StopWordsRemovingTransformer.cs	86.1% <0%> (-0.16%)	⬇️
src/Microsoft.ML.Transforms/Text/LdaTransform.cs	84.44% <0%> (-0.16%)	⬇️
... and 205 more

[eerhardt]

I think this approach is better than the previous method of using Reflection to look for a special static field on the type.

Don't forget to apply the attribute to the other places as necessary.

[antoniovs1029]

So, as discussed offline, I will open other separate issues to address the problems mentioned on Question 1 and Question 2 of this comment

Regarding Question 3 on that comment, and the answers that @yaeldekel provided, as well as suggestions made by @eerhardt offline, it will be better to not make any change on the SchemaBindableCalibratedModelParameters<,> class, since it is not used in the same way as the other CMPs (i.e. it's not used inside binary prediction transformer), and it is used in ways that might not present the same problem as the other CMPs (i.e. that they're loaded from disk using incorrect types for its internal predictor and calibrator). If in the future SBCMP presents this problem, then it might be appropriate to change it in a similar manner as the other CMPs are being modified in this PR.

So there are some minor fixes that I will want to make on this PR, and after that I will remove the Draft status, and I would appreciate for people to review this and approve it.Thanks. #Resolved

[yaeldekel]

Thank you! Can you please add a link to these new issues in this PR?

---

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

[antoniovs1029]

Thank you! Can you please add a link to these new issues in this PR?

@yaeldekel Yes, I've opened the following issues and made a reference there to this PR:

• Issue Incorrect code in BinaryModelParameters classes #4381
• Issue Create methods not being called when loading models from disk #4385

[yaeldekel]