Refactor GAM Predictor to be Public-ly Creatable #2142

rogancarr · 2019-01-14T23:50:51Z

This PR refactors the GAM trainer and predictor such that all training information remains in the trainer, and the GAM predictor is for any generic (binned) GAM.

Fixes #1948

Update: Two small bug fixes included in this PR

Fixed a serialization error when reading in GAM models written prior to 0.6. Now we issue a warning with a workaround.
Fixed a scoring error for sparse datasets.

sfilipi · 2019-01-15T07:16:15Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+
+namespace Microsoft.ML.Trainers.FastTree
+{
+    public abstract class GamModelParametersBase : ModelParametersBase<float>, IValueMapper, ICalculateFeatureContribution,


public [](start = 4, length = 6)

a line of XML in anything public..

Ivanidzo4ka · 2019-01-15T23:52:24Z

src/Microsoft.ML.FastTree/GamClassification.cs

-                verReadableCur: 0x00010001,
+                // verWrittenCur: 0x00010001, // Initial
+                // verWrittenCur: 0x00010001, // Added Intercept but collided from release 0.6-0.9
+                verWrittenCur: 0x00020001,    // Added Intercept (version revved to address collisions)


0x00020001 [](start = 31, length = 10)

I think in general we upgrade version by last digit, i.e. 0x0010002 instead of 0x00020001 #Closed

Done. #Closed

artidoro · 2019-01-15T23:55:10Z

src/Microsoft.ML.FastTree/GamClassification.cs

-        internal BinaryClassificationGamModelParameters(IHostEnvironment env, int inputLength, Dataset trainset,
-            double meanEffect, double[][] binEffects, int[] featureMap)
-            : base(env, LoaderSignature, inputLength, trainset, meanEffect, binEffects, featureMap) { }
+        public BinaryClassificationGamModelParameters(IHostEnvironment env,


BinaryClassificationGamModelParameters [](start = 15, length = 38)

Since you are making this public, could you also add XML documentation of what it does and what the parameters are?

Ivanidzo4ka · 2019-01-15T23:57:16Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// For Generalized Additive Models (GAM), the contribution of a feature is equal to the shape function for the given feature evaluated at
+        /// the feature value.
+        /// </summary>
+        public FeatureContributionCalculator FeatureContributionClaculator => new FeatureContributionCalculator(this);


FeatureContributionCalculator [](start = 15, length = 29)

this class has one internal property and one internal method.
I advice to make it internal instead of public (as class in general and as property of Gam predictor)

Can you clarify what you mean here?

This thing gonna be visible to any user. In same time if you look on content of this class, it has only internal objects.
Basically during debugging user will see FeatureContributionCalculator, but it will have zero public methods and properties.
We have that thing for a reason, but I doubt it should be public.

@artidoro Does this method need to be public to work?

Ivanidzo4ka · 2019-01-15T23:59:21Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// <returns>The bin upper bounds. May be null if this feature has no bins.</returns>
+        public double[] GetFeatureBinUpperBounds(int featureIndex)
+        {
+            Contracts.Assert(0 <= featureIndex && featureIndex < _numFeatures);


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

use Check for public methods.
Also give it proper message. #Closed

Done. #Closed

Ivanidzo4ka · 2019-01-16T00:00:27Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        private readonly double[][] _binUpperBounds;
+        private readonly double[][] _binEffects;
+        public readonly double Intercept;
+        private readonly int _numFeatures;


_numFeatures [](start = 29, length = 12)

I don't see any way for user to access this, but in same time it's used as constrain in public method.
Can we expose it with proper description? #Closed

Done. #Closed

Ivanidzo4ka · 2019-01-16T00:01:16Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// <returns>The binned weights for each feature.</returns>
+        public double[] GetFeatureWeights(int featureIndex)
+        {
+            Contracts.Assert(0 <= featureIndex && featureIndex < _numFeatures);


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

check and proper message. #Closed

artidoro · 2019-01-16T00:05:35Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// </summary>
+        /// <param name="featureIndex">The index of the feature (in the training vector) to get.</param>
+        /// <returns>The bin upper bounds. May be null if this feature has no bins.</returns>
+        public double[] GetFeatureBinUpperBounds(int featureIndex)


GetFeatureBinUpperBounds [](start = 24, length = 24)

Is there a reason why we don't want to return the upper bounds for all features with an array of arrays?
Same question for the binned weights.

No real reason. Let's add this as a separate issue.

artidoro · 2019-01-16T00:08:24Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+            double[] featureWeights;
+            if (_inputFeatureToDatasetFeatureMap.TryGetValue(featureIndex, out int j))
+            {
+                featureWeights = new double[_binUpperBounds[j].Length];


_binUpperBounds [](start = 44, length = 15)

Just checking that this is the same length as _binEffects[j]

Good catch! Should be the same and checked when defined, but not good practice. Changing to use _binEffects[j].Length.

Ivanidzo4ka · 2019-01-16T00:11:31Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        }
+
+        /// <summary>
+        /// The GAM model visualization command. Because the data access commands must access private members of


must access private members [](start = 82, length = 27)

I think it's a sign what we haven't properly design predictor.
From what I see it needs:
_inputType.ValueCount which is same as _numFeatures which I think we need to expose.
_inputFeatureToDatasetFeatureMap , _binEffects and _featureMap First two I think you use in GetFeatureBinUpperBounds and it can probably be rewritten. last one can be made as internal.

I found it's strange what we make command line thing which has more functionality than user can achieve by himself.

Point of confusion. Currently, there can be fewer shape functions built than input features because of flocking (I think this is a bug and will file one separately.)

_inputType.ValueCount is different than _numFeatures. I've rename numFeatures to NumShapeFunctions to clarify this.

inputFeaturesToDatasetFeatureMap has been renamed to _inputFeatureToShapeFunctionMap

featureMap has been renamed to shapeToInputMap. (and is private).

I found it's strange what we make command line thing which has more functionality than user can achieve by himself.

You mean the visualization command? With the new public interfaces, the user should be able to hand-edit models and create versions with the modified shapes.

_inputType.ValueCount is different than _numFeatures.

If featureToInputMap is null, I think they should be same, otherwise it's a size of featureToInputMap. Sorry, didn't properly check constructor of predictor.

You mean the visualization command? With the new public interfaces, the user should be able to hand-edit models and create versions with the modified shapes.

I just find it strange what we have nested internal command in predictor. And only reason why it's nested is because we using some internal guts of predictor. I understand what you can do hand-edits, but can user visualize model same way how our visualize command work? If not, it's probably sign what we didn't expose enough.

Ah, I see what you are saying. The visualization command is for an internal tool that may not be released. I am not sure what the plan for visualization in the .NET world is, but for now we are "visualizing" by pretty-printing, as we do in the docs. Obviously not as nice as fully interactive visualizations!

Ivanidzo4ka · 2019-01-16T00:13:48Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// <returns>The bin upper bounds. May be null if this feature has no bins.</returns>
+        public double[] GetFeatureBinUpperBounds(int featureIndex)
+        {
+            Contracts.Assert(0 <= featureIndex && featureIndex < _numFeatures);


Contracts [](start = 12, length = 9)

You have Host. #Closed

Done. #Closed

Ivanidzo4ka · 2019-01-16T00:13:56Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+        /// <returns>The binned weights for each feature.</returns>
+        public double[] GetFeatureWeights(int featureIndex)
+        {
+            Contracts.Assert(0 <= featureIndex && featureIndex < _numFeatures);


Contracts [](start = 12, length = 9)

Please use Host. #Closed

Done. #Closed

Ivanidzo4ka · 2019-01-16T00:14:35Z

using System;

no header???!!!11 #Closed

Refers to: src/Microsoft.ML.FastTree/GamModelParameters.cs:1 in add0cc8. [](commit_id = add0cc8, deletion_comment = False)

Ivanidzo4ka · 2019-01-16T00:17:26Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+
+        private double GetBinEffect(int featureIndex, double featureValue)
+        {
+            Contracts.Assert(0 <= featureIndex && featureIndex < _numFeatures);


Contracts [](start = 12, length = 9)

Please change Contracts to Host where it possible. #Closed

Done. #Closed

Ivanidzo4ka · 2019-01-16T00:18:25Z

src/Microsoft.ML.FastTree/GamModelParameters.cs

+                gtChild.Insert(0, right);
+                return --internalNodeId;
+            }
+        }


new line #Closed

Done #Closed

Ivanidzo4ka

…ion into the trainer, and make the GAM predictor generic.

…ing classes; fixing a serialization error; fixing an error in the sparsity calculation.

TomFinley · 2019-01-17T20:17:08Z

Hi @rogancarr I'm having trouble getting started reading this, since I can't quite find where the predictors have been changed to use generics?

rogancarr · 2019-01-17T23:37:43Z

Hi @rogancarr I'm having trouble getting started reading this, since I can't quite find where the predictors have been changed to use generics?

Hi @TomFinley! Apologies for the confusion. I renamed this PR to make more sense. This PR changes the GAM predictor s.t. it can be created with the output of any GAM training (with binned results). This is part of the move to create public constructors for our ModelParameters classes.

It does not have anything to do with generics in the programming language sense.

artidoro

rogancarr requested a review from najeeb-kazmi January 14, 2019 23:50

sfilipi reviewed Jan 15, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 15, 2019

View reviewed changes

artidoro reviewed Jan 15, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 15, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 16, 2019

View reviewed changes

artidoro reviewed Jan 16, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 16, 2019

View reviewed changes

rogancarr force-pushed the gam_generic_modelparameters branch from add0cc8 to 1f65d26 Compare January 16, 2019 22:10

Ivanidzo4ka approved these changes Jan 16, 2019

View reviewed changes

Rogan Carr added 5 commits January 16, 2019 14:33

Refactoring the GAM Trainer / Predictor to move all Training informat…

a28e6b8

…ion into the trainer, and make the GAM predictor generic.

Put model parameters in their own file

58ae0dc

Adding XML Docs to public functions; making accessors public and seal…

01c5410

…ing classes; fixing a serialization error; fixing an error in the sparsity calculation.

Updating in response to PR comments; fixing failing tests.

debbb76

Rebasing to match the new history.

4179af2

rogancarr force-pushed the gam_generic_modelparameters branch from 1f65d26 to 4179af2 Compare January 16, 2019 22:43

rogancarr changed the title ~~Refactor GAM Predictor to be Generic~~ Refactor GAM Predictor to be Public-ly Creatable Jan 17, 2019

This was referenced Jan 18, 2019

GAMs only allow getting one feature at a time #2187

Closed

Test GAM Public APIs #2188

Merged

artidoro approved these changes Jan 18, 2019

View reviewed changes

rogancarr merged commit 240faeb into dotnet:master Jan 22, 2019

rogancarr deleted the gam_generic_modelparameters branch January 22, 2019 21:21

dotnet locked as resolved and limited conversation to collaborators Mar 25, 2022

Refactor GAM Predictor to be Public-ly Creatable #2142

Refactor GAM Predictor to be Public-ly Creatable #2142

Conversation

rogancarr commented Jan 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jan 15, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jan 15, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Jan 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka commented Jan 16, 2019 • edited

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka Jan 16, 2019 • edited

Choose a reason for hiding this comment

rogancarr Jan 16, 2019 • edited by Ivanidzo4ka

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

TomFinley commented Jan 17, 2019

rogancarr commented Jan 17, 2019

artidoro left a comment

Choose a reason for hiding this comment

rogancarr commented Jan 14, 2019 •

edited

Ivanidzo4ka Jan 15, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka Jan 15, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka Jan 16, 2019 •

edited

artidoro Jan 16, 2019 •

edited

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka commented Jan 16, 2019 •

edited

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka

Ivanidzo4ka Jan 16, 2019 •

edited

rogancarr Jan 16, 2019 •

edited by Ivanidzo4ka