OneVersusAllModelParameters Strongly Typed by michaelgsharp · Pull Request #4013 · dotnet/machinelearning

michaelgsharp · 2019-07-17T21:31:11Z

We used to remove all Type information when we constructed our OneVersusAllModelParameters. This prevented the users from access the inner model without run-time casting.

This PR makes the OneVersusAllModelParameters strongly typed, and adds a strongly typed version of the OneVersusAllTrainer as it uses the OneVersusAllModelParameters. This change is no longer a breaking change to the current public api.

eerhardt · 2019-07-18T15:44:26Z

    /// </summary>
-    [BestFriend]
-    internal enum PredictionKind
+    public enum PredictionKind


These types aren't intended to be public. Can you keep them internal?

Unfortunately not if we want the OneVersusAllModelParameters to be strongly typed and keep the backwards api compatibility. Since the original type was of IPredictorProducing<float>, once I type it that way then IPredictorProducing<T>, IPredictor, and PredictionKind have to all become public as well. The only other thought I had was that maybe I could change the type of the backwards compatible OneVersusAllModelParameters to be of type object instead of IPredictorProducing<T>. This will still keep the backwards API compatibility and would allow me to make those classes internal again.

Another option would be to not have the base class be generic. Have class OneVersusAllModelParametersBase and class OneVersusAllModelParameters : OneVersusAllModelParametersBase and class OneVersusAllModelParameters<T> : OneVersusAllModelParametersBase.

These IPredictor types are not meant to be public. We explicitly made them internal in 1.0 because they shouldn't be exposed to the user. So we will have to find another way here.

I thought about that approach initially, but there are enough Strongly typed parameters in there I didn't think it would work. Let me test it and see what I can do there. If not, we can always change the type to object since that will work and not change anything about the api.

Alright, I have made the change following your suggestions, so now the interfaces are back to being internal. Thanks for your feedback!

PredictionKind should remain internal.

eerhardt · 2019-07-18T15:49:24Z

+    /// <summary>
+    /// Class that holds the static create methods for the <see cref="OneVersusAllModelParameters"/> classes.
+    /// </summary>
+    public class OneVersusAllModelParametersBuilder {


This class doesn't need to be public. It doesn't contain any public members.

Also, it can be marked as a static class.

Good points. I will change those.

eerhardt · 2019-07-18T15:52:02Z

+    /// This cannot be used to turn a non calibrated binary classification estimator into its calibrated version. If that is required, use <see cref="OneVersusAllTrainerTyped{TSubPredictor, TCalibrator}"/> instead.
+    /// </summary>
+    /// <typeparam name="T"></typeparam>
+    public sealed class OneVersusAllTrainerTyped<T> : OneVersusAllTrainerBase<OneVersusAllModelParametersBase<T>> where T : class


I'm not sure having Typed at the end of this class name is the best. Why can't it just be OneVersusAllTrainer<T>?

The problem is that the only difference between the original method and this one are the optional parameters. So if people are only specifying the required parameters, which is the most common case in our code base, then C# doesn't know which method to call. Isn't that a binary breaking change? So I chose to make this method have a different name. Do you have any other ideas on how to resolve it?

Same question about the other method I added in. I called it OneVersusAllUnCalibratedToCalibratedTyped, but since it has 2 type parameters that have to be manually specified, its name can be anything and it wont have conflicts. I just named it this to be very clear on what it does, though I am happy to change it as well if you have thoughts on this too.

This isn't a method, this is a class name. The class name and the method don't need to match 1-1.

If we need to suffix the method name with Typed or similar, that is fine. But the class doesn't need to have the suffix on it.

I see. I was thinking of the Trainers Catalog when I responded. I have fixed the class names and will push those changes. Thanks!

justinormont · 2019-07-19T10:13:42Z

+        ///  [!code-csharp[OneVersusAll](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/MulticlassClassification/OneVersusAll.cs)]
+        /// ]]></format>
+        /// </example>
+        public static OneVersusAllTrainer<TModel> OneVersusAllTyped<TModel>(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog,


Have you tested the new OneVersusAllTyped to see that you can match the scores of the existing one?

Yep! I added and modified the tests in the OvaTest.cs to show that the scores match exactly.

The test which directly compares the score of the non-typed to the newly created typed version, is the perfect unit test. Thanks.

justinormont · 2019-08-02T18:42:17Z

+
+            Assert.Equal(metrics.MicroAccuracy, metricsTyped.MicroAccuracy);
        }
+                //.Append(ML.MulticlassClassification.Trainers.OneVersusAllUnCalibratedToCalibratedTyped<LinearBinaryModelParameters, PlattCalibrator>(sdcaTrainer))


Extraneous comment.

justinormont · 2019-08-02T19:08:10Z

+        /// the binary classification estimator specified by <paramref name="binaryEstimator"/>.This method works with binary classifiers that
+        /// are not calibrated and need to be calibrated before use. Due to the type of estimator changing (from uncalibrated to calibrated), you must manually
+        /// specify both the type of the model and the type of the calibrator. If your classifier is already calibrated or it does not need to be, use the
+        /// <see cref="OneVersusAllTyped{TModel}"/> method instead.


May want to explicitly state:
If you want to retrieve the model parameters, you will want to use this strongly typed OneVersusAllTrainer.

And perhaps a note on the non-strongly typed which says similar.

yaeldekel · 2019-08-04T08:25:12Z

        {
            Host.Check(predictor is IParameterMixer<float>, "Predictor does not implement " + nameof(IParameterMixer<float>));
            Host.Check(calibrator is IParameterMixer, "Calibrator does not implement " + nameof(IParameterMixer));
            Host.Assert(predictor is IPredictorWithFeatureWeights<float>);


Assert [](start = 17, length = 6)

Can this be a check instead of assert?

On second thought, why does this class need to have a generic type?

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

This class needs to be generic to get the typed predictor and calibrator out of it. Otherwise OVA would just return this and it wouldn't be useful at all as far as getting the model goes. And I didn't do anything with the assert, so I am not sure why they did it that way in the beginning.

This specific class is internal anyway, so if OVA uses it users will not be able to get the typed calibrator anyway. But I don't think OVA uses this class, does it?

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

So when OVA is returning the sub model parameters, if the model is calibrated it will return an instance of the CalibratedModelParametersBase. This class is public, and still allows access to the model parameters and the calibrator used for the training. Any changes made in this file were to facilitate this CalibratedModelParametersBase and returning the correct type, while still keeping the derived classes abstract.

…R comments

codemzs · 2019-10-03T00:54:17Z

@michaelgsharp Closing this because this is not actively worked on.

michaelgsharp requested review from artidoro, codemzs, eerhardt, justinormont and wschin July 17, 2019 21:31

michaelgsharp self-assigned this Jul 17, 2019

michaelgsharp mentioned this pull request Jul 17, 2019

OneVersusAllModelParameters Strongly Typed #3928

Closed

eerhardt reviewed Jul 18, 2019

View reviewed changes

justinormont reviewed Jul 19, 2019

View reviewed changes

justinormont reviewed Aug 2, 2019

View reviewed changes

yaeldekel reviewed Aug 4, 2019

View reviewed changes

michaelgsharp added 16 commits August 5, 2019 11:03

base class in place

c43302e

small changes

d82fc6e

working but has copy pasted code

06637ca

changes per feedback

15401d2

fixes in methods

5af4c01

temp checkpoint

31b9aa8

all working. Needs more test cases

82ec5e2

constructor changes

ece3bd0

more tests added

b56fe0e

fixes for API compat based on Artidoro's comments

cd04fd9

final changes for testing

2f0fe03

expanded comments

d488049

small formatting fixes

46fab04

removed unused import. Changed type of class to static internal per P…

238bf6c

…R comments

class name changes from PR comments

8f851fa

restructured OVAModelParameters. changed interfaces back to internal

32ddb54

michaelgsharp added 2 commits August 5, 2019 11:03

added tests to test 'EstimatorCore' with the new typed classes

cb3ef0f

comment fixes

d676d46

michaelgsharp force-pushed the OvaTyping branch from 69db84c to d676d46 Compare August 5, 2019 18:03

small fix from PR comment

cf9ef42

codemzs closed this Oct 3, 2019

michaelgsharp deleted the OvaTyping branch December 3, 2019 22:38

ghost locked as resolved and limited conversation to collaborators Mar 21, 2022

Conversation

michaelgsharp commented Jul 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelgsharp Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codemzs commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michaelgsharp Jul 18, 2019 •

edited

Loading