Added Feature Contribution Calculation Transform #1677

ganik · 2018-11-20T00:05:57Z

fixes #1644

Merge

ganik · 2018-11-20T00:07:22Z

src/Microsoft.ML.Data/Scorers/WhatTheFeatureScorer.cs

+    /// The What The Feature scorer is superset of a generic scorer.
+    /// It outputs score columns from Generic Scorer plus for given features provides vector of corresponding feature contributions.
+    /// </summary>
+    public sealed class WhatTheFeatureScorerTransform


WhatTheFeatureScorerTransform [](start = 24, length = 29)

Need different name, pls suggest #Resolved

Let's go with FeatureImportanceCalculationTransform for now. #Resolved

One last naming change: Please name this with the prefix FeatureContribution rather than FeatureImportance. This will make it less ambiguous what it's calculating. #Resolved

ganik · 2018-11-20T00:07:59Z

src/Microsoft.ML.Data/Scorers/WhatTheFeatureScorer.cs

+            // REVIEW: the scorer currently ignores the 'suffix' argument from the base class. It should respect it.
+        }
+
+        public static IDataScorerTransform Create(IHostEnvironment env, Arguments args, IDataView data, ISchemaBoundMapper mapper, RoleMappedSchema trainSchema)


IDataScorerTransform [](start = 22, length = 20)

Will add samples & docs #Resolved

rogancarr · 2018-11-20T00:10:11Z

@ganik Here is an example usage from @Zruty0

var trainer = ml….FastTree()
var trees = trainer.Fit(data);
var WtfModel = new WtfTransformer(trees.Model, trees.FeatureColumn, otherWtfScorerArgs);

@Zruty0 What should the high-level API look like, starting from the mlContext object? #Resolved

rogancarr · 2018-11-20T20:52:26Z

@ganik Here is an example usage from @Zruty0
var trainer = ml….FastTree()
var trees = trainer.Fit(data);
var WtfModel = new WtfTransformer(trees.Model, trees.FeatureColumn, otherWtfScorerArgs);
@Zruty0 What should the high-level API look like, starting from the mlContext object?

@ganik I had an offline conversation with @Zruty0 about this. We don't need to put the WtfScorer into the Catalog. We can use direct instantiation. If we change our minds, the high-level APIs will come out post 0.8. #Resolved

Merge

rogancarr · 2018-11-26T18:44:58Z

src/Microsoft.ML.Data/Scorers/FeatureImportanceCalculationTransform.cs

+using Microsoft.ML.Runtime.Model;
+using Microsoft.ML.Runtime.Numeric;
+
+[assembly: LoadableClass(typeof(IDataScorerTransform), typeof(FeatureImportanceCalculationTransform), typeof(FeatureImportanceCalculationTransform.Arguments),


Please remove all references to "WTF" #Resolved

rogancarr · 2018-11-26T18:45:27Z

src/Microsoft.ML.Data/Scorers/FeatureImportanceCalculationTransform.cs

+    /// The Feature Importance scorer is superset of a generic scorer.
+    /// It outputs score columns from Generic Scorer plus for given features provides vector of corresponding feature contributions.
+    /// </summary>
+    public sealed class FeatureImportanceCalculationTransform


FeatureImportanceCalculationTransform => FeatureContributionCalculationTransform #Resolved

TomFinley · 2018-11-26T21:11:52Z

test/Microsoft.ML.Tests/FeatureImportanceTests.cs

+                Bottom = 10,
+                Top = 10
+            };
+            var output = FeatureImportanceCalculationTransform.Create(Env, args, data, model.Model, model.FeatureColumn);


FeatureImportanceCalculationTransform.Create [](start = 25, length = 44)

Hi @ganik thanks for working on this. Please see #581 and #754, and read.

Central to the idea, as we see in #581, is that we are making the things that trains a transform, is a transform, and is the transformed data, three separate things (IEstimator, ITransformer, IDataView) rather than having them conflated in one big huge thing as we have done previously (e.g., the IDataLoader and IDataTransform interfaces).

We have had, by now, some dozens of issues and PRs meant for the work we are doing to do the conversion from IDataTransform based scheme, to the IEstimator and ITransformer based scheme, as I think you are aware since you have done some of this conversion yourself. (E.g., your work in #1254 where you successfully converted some of the old timeseries transformers over to estimator land.)

So let's review this idiom here. This idiom where you've created the transform that is also simultaneously the data is an old concept that was used in the internal tool, but we do not want people to be exposed to because it is incredibly confusing unless you're somehow expecting it. So: if you want to ship this, we should not expose this to users yet, until we structure it properly.

We might consider staging the work so that we internally ship this scorer, but not announce it in any way. #ByDesign

totally agreed. The right way to go is to implement estimator API. This is scheduled for 0.9. This particular change is a staged change that doesn't need to get announced.

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

This is great feedback and a good plan. Getting this in 0.8 with the current design would let us address issues like #1675 without needing to wait for the refactoring to be finished. #Resolved

rogancarr · 2018-11-26T21:57:58Z

I've added a sample showing how this can be used. #Resolved

Ivanidzo4ka · 2018-11-27T00:41:54Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+            }
+        }
+
+        private sealed class WtfSchema : ISchema


Wtf [](start = 29, length = 3)

I would suggest to search "Wtf" in this code, and replace it with something else. #Resolved

I suggest FeatureContribution. #Resolved

…nto ganik/wf

Ivanidzo4ka · 2018-11-27T21:11:45Z

src/Microsoft.ML.Data/Prediction/Calibrator.cs

        {
            // REVIEW: checking this a bit too late.
-            Host.Check(_whatTheFeature != null, "Predictor does not implement IWhatTheFeatureValueMapper");
-            return _whatTheFeature.GetWhatTheFeatureMapper<TSrc, TDst>(top, bottom, normalize);
+            Host.Check(_featureContribution != null, "Predictor does not implement IFeatureContributionMapper");


IFeatureContributionMapper [](start = 83, length = 26)

nameof maybe? #Resolved

Ivanidzo4ka · 2018-11-27T21:13:47Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+            [Argument(ArgumentType.AtMostOnce, HelpText = "Number of bottom contributions", SortOrder = 2)]
+            public int Bottom = 10;
+
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Whether or not output of Features' contribution should be normalized", ShortName = "norm", SortOrder = 3)]


' [](start = 92, length = 1)

that apos is inconsistent between norm and stringify #Resolved

TomFinley · 2018-11-27T21:14:19Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+using Microsoft.ML.Runtime.Numeric;
+
+[assembly: LoadableClass(typeof(IDataScorerTransform), typeof(FeatureContributionCalculationTransform), typeof(FeatureContributionCalculationTransform.Arguments),
+    typeof(SignatureDataScorer), "Feature Importance Scorer", "wtf", "FeatureImportanceCalculationScorer", MetadataUtils.Const.ScoreColumnKind.FeatureContribution)]


"wtf" [](start = 62, length = 5)

I could be wrong, but I seem to recall that we would change the user facing names of these things. I think we ought to have SignatureLoadModel remain the same for backwards compatibility. #Resolved

"Feature Importance Scorer", "wtf", "FeatureImportanceCalculationScorer"=>"Feature Contribution Transform", "FeatureContributionCalculationTransform"` #Resolved

Ivanidzo4ka · 2018-11-27T21:14:33Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+using Microsoft.ML.Runtime.Numeric;
+
+[assembly: LoadableClass(typeof(IDataScorerTransform), typeof(FeatureContributionCalculationTransform), typeof(FeatureContributionCalculationTransform.Arguments),
+    typeof(SignatureDataScorer), "Feature Importance Scorer", "wtf", "FeatureImportanceCalculationScorer", MetadataUtils.Const.ScoreColumnKind.FeatureContribution)]


wtf [](start = 63, length = 3)

I think @[email protected] is fine to get rid of "wtf" from command line and replace with something else like "FI" or "FIS" #Resolved

TomFinley · 2018-11-27T21:14:41Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+    {
+        // Apparently, loader signature is limited in length to 24 characters.
+        internal const string MapperLoaderSignature = "WTFBindable";
+        private const string LoaderSignature = "WTFScorer";


private const string LoaderSignature = "WTFScorer"; [](start = 8, length = 51)

So, what is this for? #Resolved

wschin · 2018-11-27T21:47:56Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+namespace Microsoft.ML.Runtime.Data
+{
+    /// <summary>
+    /// The Feature Importance scorer is superset of a generic scorer.


I feel this doc string is not long enough. External users who reads this file might not know generic scorer and other concepts in ML.NET. #Resolved

@rogancarr could you take care of this? thx

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

I've added a basic description to the transform. As per @shauheen 's, advice, let's focus on the code now and finalize the doc strings after code complete. #Resolved

Ivanidzo4ka · 2018-11-27T23:49:40Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+                            sb.Remove(sb.Length - 2, 2);
+                        }
+
+                        dst = new ReadOnlyMemory<char>(sb.ToString().ToCharArray());


new ReadOnlyMemory(sb.ToString().ToCharArray()); [](start = 30, length = 54)

sb.ToString.AsMemory() #Resolved

Ivanidzo4ka · 2018-11-27T23:50:09Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+                _env.Assert(count > index || count == 0 && slotNames.Length > index);
+                var slotName = slotNames.GetItemOrDefault(index);
+                return slotName.IsEmpty
+                    ? new ReadOnlyMemory<char>($"f{index}".ToCharArray())


new ReadOnlyMemory($"f{index}".ToCharArray()) [](start = 22, length = 51)

$"f{index}".AsMemory() #Resolved

Ivanidzo4ka · 2018-11-27T23:52:26Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+                        {
+                            _env.Assert(sb.Length >= 2);
+                            sb.Remove(sb.Length - 2, 2);
+                        }


a) why we doing this?
b) wouldn't it be easier to have if (sb.Length>=2) condition? #WontFix

its not the same, assert here, so if its bigger than 0 then it should be bigger or equal than 2. Dont want to change this logic since this is how it is originally in internal repo

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

Ivanidzo4ka · 2018-11-27T23:55:04Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+                        map(in features, ref contributions);
+                        var indices = new Span<int>();
+                        var values = new Span<float>();
+                        (contributions.IsDense ? Utils.GetIdentityPermutation(contributions.Length).AsSpan() : contributions.GetIndices()).CopyTo(indices);


Utils.GetIdentityPermutation(contributions.Length).AsSpan() [](start = 49, length = 59)

what is going on here? #Resolved

Ivanidzo4ka

TomFinley

Thanks @ganik

singlis · 2018-11-28T00:16:29Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+            if (args.Top <= 0 || args.Top > MaxTopBottom)
+                throw env.Except($"Number of top contribution must be in range (0,{MaxTopBottom}]");
+            if (args.Bottom <= 0 || args.Bottom > MaxTopBottom)
+                throw env.Except($"Number of bottom contribution must be in range (0,{MaxTopBottom}]");


minor nit, I would move this into a helper function. ValidateTopBottomArguments? #WontFix

singlis · 2018-11-28T00:22:31Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+
+            public readonly IFeatureContributionMapper Predictor;
+            public readonly ISchemaBindableMapper GenericMapper;
+            public readonly bool Stringify;


minor nit - I would group all the properties together (above VersionInfo). #Resolved

singlis · 2018-11-28T00:27:39Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+
+                Func<IRow, int, VBuffer<ReadOnlyMemory<char>>, ValueGetter<ReadOnlyMemory<char>>> del = GetTextValueGetter<int>;
+                var meth = del.GetMethodInfo().GetGenericMethodDefinition().MakeGenericMethod(typeSrc.RawType);
+                return (Delegate)meth.Invoke(this, new object[] { input, colSrc, slotNames });


I believe you can use Utils.MarshalInvoke here since there is only one generic argument that is needed in the function. #WontFix

singlis · 2018-11-28T00:42:07Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+    /// ]]>
+    /// </format>
+    /// </example>
+    public sealed class FeatureContributionCalculationTransform


Im guessing there will need to be an estimator too at some point. What would that be called? Does that affect the name of the transform at all. Note for Estimators have the format of ItemEstimator (ColumnCopyingEstimator). #ByDesign

Yes there will be, name yet to be determined. Dont think name will be affected, also should be ok to change transform name if needed

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

singlis

eerhardt · 2018-12-03T15:56:41Z

src/Microsoft.ML.Data/Scorers/FeatureContributionCalculationTransform.cs

+                    {
+                        featureGetter(ref features);
+                        map(in features, ref contributions);
+                        var indices = new Span<int>();


This code is pretty wrong. You are making an empty span, and then trying to copy things into it (which certainly will throw an exception if the thing you are trying to copy has any elements).

Are we testing this code? I don't see how it could be working.

Would the tests cover this part of the code?

ganik and others added 9 commits October 10, 2018 14:49

Merge pull request #1 from dotnet/master

f5e421e

Merge

Merge pull request #2 from dotnet/master

13f5f76

Merge

Merge pull request #3 from dotnet/master

1aa7f55

Merge

Merge pull request #4 from dotnet/master

546f2fa

Merge

Merge pull request #5 from dotnet/master

b8785af

Merge

Merge pull request #6 from dotnet/master

8bb723f

Merge

Merge pull request #7 from dotnet/master

0f455a7

Merge

Merge pull request #8 from dotnet/master

0198dd8

Merge

initial draft

f906004

ganik commented Nov 20, 2018

View reviewed changes

fix build

9cee681

ganik and others added 6 commits November 25, 2018 18:52

rename

26b6286

rename

fbd2177

Merge pull request #9 from dotnet/master

af96dff

Merge

Merge branch 'master' into ganik/wf

2dcd1fd

add test

814bf9f

fix comment

3455ef1

ganik changed the title ~~WIP: Adding WhatTheFeature Scorer~~ Added Feature Importance Calculation Transform Nov 26, 2018

rogancarr reviewed Nov 26, 2018

View reviewed changes

TomFinley reviewed Nov 26, 2018

View reviewed changes

ganik and others added 2 commits November 26, 2018 13:30

fix comments

99d7bcb

Adding a sample for using FeatureContributionCalculatorTransform

68c9245

Ivanidzo4ka reviewed Nov 27, 2018

View reviewed changes

ganik changed the title ~~Added Feature Importance Calculation Transform~~ Added Feature Contribution Calculation Transform Nov 27, 2018

rename

fd2cee9

Merge branch 'ganik/wf' of https://github.com/ganik/machinelearning i…

d131959

…nto ganik/wf

Ivanidzo4ka reviewed Nov 27, 2018

View reviewed changes

TomFinley reviewed Nov 27, 2018

View reviewed changes

Ivanidzo4ka reviewed Nov 27, 2018

View reviewed changes

TomFinley reviewed Nov 27, 2018

View reviewed changes

wschin reviewed Nov 27, 2018

View reviewed changes

ganik and others added 3 commits November 27, 2018 13:54

fix comments

da1b462

fix comments

9af3892

Updated the doc string.

4bacc8f

Ivanidzo4ka reviewed Nov 27, 2018

View reviewed changes

Ivanidzo4ka approved these changes Nov 28, 2018

View reviewed changes

TomFinley approved these changes Nov 28, 2018

View reviewed changes

singlis reviewed Nov 28, 2018

View reviewed changes

singlis approved these changes Nov 28, 2018

View reviewed changes

fix comments

754fbc6

rogancarr merged commit 200eef7 into dotnet:master Nov 28, 2018

eerhardt reviewed Dec 3, 2018

View reviewed changes

ganik deleted the ganik/wf branch December 20, 2018 04:23

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

Added Feature Contribution Calculation Transform #1677

Added Feature Contribution Calculation Transform #1677

Conversation

ganik commented Nov 20, 2018

ganik Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

rogancarr Nov 20, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

rogancarr Nov 26, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

rogancarr commented Nov 20, 2018 • edited by ganik Loading

rogancarr commented Nov 20, 2018 • edited by ganik Loading

rogancarr Nov 26, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

rogancarr Nov 26, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley Nov 26, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Nov 27, 2018

Choose a reason for hiding this comment

rogancarr Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

rogancarr commented Nov 26, 2018 • edited by ganik Loading

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

rogancarr Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

rogancarr Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

TomFinley Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

wschin Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Nov 27, 2018

Choose a reason for hiding this comment

rogancarr Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Nov 28, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Nov 27, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

singlis Nov 28, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

singlis Nov 28, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

singlis Nov 28, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

singlis Nov 28, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

ganik Nov 28, 2018

Choose a reason for hiding this comment

singlis left a comment

Choose a reason for hiding this comment

eerhardt Dec 3, 2018

Choose a reason for hiding this comment

rogancarr Dec 3, 2018

Choose a reason for hiding this comment

ganik Nov 20, 2018 •

edited

Loading

rogancarr Nov 20, 2018 •

edited by ganik

Loading

rogancarr Nov 26, 2018 •

edited by ganik

Loading

ganik Nov 20, 2018 •

edited

Loading

rogancarr commented Nov 20, 2018 •

edited by ganik

Loading

rogancarr commented Nov 20, 2018 •

edited by ganik

Loading

rogancarr Nov 26, 2018 •

edited by ganik

Loading

rogancarr Nov 26, 2018 •

edited by ganik

Loading

TomFinley Nov 26, 2018 •

edited by ganik

Loading

rogancarr Nov 27, 2018 •

edited by ganik

Loading

rogancarr commented Nov 26, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

rogancarr Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

TomFinley Nov 27, 2018 •

edited by ganik

Loading

rogancarr Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

TomFinley Nov 27, 2018 •

edited by ganik

Loading

wschin Nov 27, 2018 •

edited by ganik

Loading

rogancarr Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

Ivanidzo4ka Nov 27, 2018 •

edited by ganik

Loading

singlis Nov 28, 2018 •

edited by ganik

Loading

singlis Nov 28, 2018 •

edited by ganik

Loading

singlis Nov 28, 2018 •

edited by ganik

Loading

singlis Nov 28, 2018 •

edited by ganik

Loading