making GetCoefficientStatistics public #1979

sfilipi · 2018-12-28T06:32:58Z

Fixes #1977 by making method GetCoefficientStatistics public.

TomFinley · 2018-12-28T17:54:57Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

    /// </summary>
    public readonly struct CoefficientStatistics
    {
        public readonly string Name;


public readonly string Name; [](start = 8, length = 28)

As discussed, if this is to be part of our public API we should probably as part of this PR make this entire structure a bit less goofy. If I were to trace back the root of what makes this structure a confusing structure, I think this choice here -- having the name of the slot instead of just the slot index, is the original sin. (There are lots of other really strange choices, but I think much of the evil and confusion stems from here.)

This should have just been The fact that this is a Nameinstead of just aSlotIndex` is the cause for an enormous amount of confusion in this structure. This requires that we link it with the schema column for features, but from an API perspective this is just something that could have easily (in fact, more easily) be done by the user themselves. As it is, the structure is offering "convenience" but in a way that inevitably leads to a more complicated API.

Once you do that, restructuring the odd API below (which I'll comment more on in the test file) should become more approachable.

This is not to say that it might be convenient to have an internal utility method that makes saving as text more convenient for the LinearModelParameters and whatnot, but confusing the needs of the text exporter (which is not part of our public API) with what we actually do in the public API was a misstep. (Not your misstep to be clear, since this code has existed, I think, more or less forever, but still something we ought to correct now that we are making it part of the publci API.)

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

TomFinley · 2018-12-28T17:56:21Z

test/Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs

-            var linearModel = transformerChain.LastTransformer.Model.SubPredictor as LinearBinaryModelParameters;
+            var linearModel = transformer.LastTransformer.Model.SubPredictor as LinearBinaryModelParameters;
            var stats = linearModel.Statistics;
            LinearModelStatistics.TryGetBiasStatistics(stats, 2, out float stdError, out float zScore, out float pValue);


LinearModelStatistics.TryGetBiasStatistics(stats, 2, out float stdError, out float zScore, out float pValue); [](start = 12, length = 109)

This is the first thing that does not make much sense to me. It seems like linearModel.Statistics should offer similar accessors through an instance method. (Why out parameters? Why not just properties on the original structure?) This is just really complicated.

+1

As Tom pointed out, the API for getting statistics needs some cleanup. linearModel.Statistics or some other simple 1-liner should be the only code a user needs.

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

TomFinley · 2018-12-28T17:58:03Z

test/Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs

+
+            var scoredData = transformer.Transform(dataView);
+
+            var coeffcients  = stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100);


stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100); [](start = 31, length = 80)

This is the second thing that does not make sense to me. Again should just be an instance method. As discussed also, the complication that this code itself is responsible for extracting out the slot names was a mistake. Anticipating some people's reaction, if the reaction from some people would be "well it's more convenient," then the correct remediation is to just make slot names easier to access (somehow), definitely not to "solve" the problem here as is done.

TomFinley · 2018-12-28T17:58:32Z

test/Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs

+
+            var coeffcients  = stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100);
+
+            Assert.Equal(19, coeffcients.Length);


coeffcients [](start = 29, length = 11)

Not a big deal at all since it's just a test, but while you're at it, this is a typo. coeffcients => coefficients. #Resolved

sfilipi · 2019-01-03T21:04:26Z

@TomFinley @shmoradims I am in the process of doing the suggested refactoring, and figured that i should go a bit further: break down this class in tow parts: one that gives basic stats - used by MultiClass and LR in the case when the extended cals are not required, and another class that offers the extended stats in top of the previous.

This will be a bit more complex of a change, since it will require loading modes backwards etc; i am not confident that if i publish the changes today/tomorrow the next round of review will be final and also it will be a larger change to cherry pick.

Are you guys ok with checking in this PR, since GetCoefficientStatistics needs to be public, and getting the rest of the changes on the next PR, but in 0.10?

TomFinley

Are you guys ok with checking in this PR, since GetCoefficientStatistics needs to be public, and getting the rest of the changes on the next PR, but in 0.10?

Hmmmm. OK. 😄 But if you could write an issue to capture the problems with current public surface so someone (maybe you?) can clean up later that would make me feel better.

sfilipi · 2019-01-03T21:52:22Z

thanks Tom. Created #2010 with full description of the other issues with it. Assigned to self.

Ivanidzo4ka

Ivanidzo4ka · 2019-01-03T22:04:41Z

test/Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using System.Linq;


Why we need Linq here?

Ivanidzo4ka · 2019-01-03T22:05:22Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs


        /// <summary>
        /// Gets the coefficient statistics as an object.
        /// </summary>


Need better summary than this one.

Ivanidzo4ka · 2019-01-03T22:06:25Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

        /// Gets the coefficient statistics as an object.
        /// </summary>
-        internal CoefficientStatistics[] GetCoefficientStatistics(LinearBinaryModelParameters parent, RoleMappedSchema schema, int paramCountCap)
+        public CoefficientStatistics[] GetCoefficientStatistics(LinearBinaryModelParameters parent, Schema.Column featureColumn, int paramCountCap)


paramCountCap [](start = 133, length = 13)

this isn't great name. Param is such a generic term, I have no idea what it's capping.

shmoradims

Ivanidzo4ka · 2019-01-03T22:21:19Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

        public long TrainingExampleCount => _trainingExampleCount;

-        public Single Deviance => _deviance;
+        public float Deviance => _deviance;


_deviance [](start = 33, length = 9)

why we need _deviance?

Ivanidzo4ka · 2019-01-03T22:22:11Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

+        public float Deviance => _deviance;

-        public Single NullDeviance => _nullDeviance;
+        public float NullDeviance => _nullDeviance;


_nullDeviance [](start = 37, length = 13)

I don't see any reason for _nullDeviance to exist.

Ivanidzo4ka · 2019-01-03T22:22:23Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

+        public float Deviance => _deviance;

-        public Single NullDeviance => _nullDeviance;
+        public float NullDeviance => _nullDeviance;


NullDeviance [](start = 21, length = 12)

Need proper comment

Ivanidzo4ka · 2019-01-03T22:22:34Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

        public long TrainingExampleCount => _trainingExampleCount;

-        public Single Deviance => _deviance;
+        public float Deviance => _deviance;


Deviance [](start = 21, length = 8)

need proper comment.

Ivanidzo4ka · 2019-01-03T22:29:14Z

src/Microsoft.ML.StandardLearners/Standard/ModelStatistics.cs

            _paramCount = paramCount;
            _trainingExampleCount = trainingExampleCount;
            _deviance = deviance;
            _nullDeviance = nullDeviance;


none of _variable from that block should exist.
Just make public readonly version of it, and use it. #Resolved

Ivanidzo4ka · 2019-01-03T22:33:57Z

test/Microsoft.ML.Tests/TrainerEstimators/LbfgsTests.cs

+
+            var scoredData = transformer.Transform(dataView);
+
+            var coeffcients  = stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100);


GetCoefficientStatistics [](start = 37, length = 24)

I find it strange to have
TryGetBiasStatistics which accept LinearModelStatistics object
and GetCoefficientStatistics which are part of LinearModelStatistics object.
a) relationship to LinearModelStatistics object
b) why one is Try, and other is just Get?
#Resolved

Yep, ugly. I am fixing all of it now, working on #2010

Markign as resolved since #2010 exists.

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

making GetCoefficientStatistics public

8c96da7

sfilipi requested review from TomFinley, codemzs and shmoradims December 28, 2018 06:33

sfilipi self-assigned this Dec 28, 2018

sfilipi added API Issues pertaining the friendly API enhancement New feature or request and removed API Issues pertaining the friendly API labels Dec 28, 2018

sfilipi added this to the 1218 milestone Dec 28, 2018

TomFinley reviewed Dec 28, 2018

View reviewed changes

TomFinley approved these changes Jan 3, 2019

View reviewed changes

sfilipi mentioned this pull request Jan 3, 2019

LinearModelStatistics API usage cleanup #2010

Closed

Ivanidzo4ka approved these changes Jan 3, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 3, 2019

View reviewed changes

shmoradims approved these changes Jan 3, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 3, 2019

View reviewed changes

sfilipi merged commit 85db282 into dotnet:master Jan 3, 2019

sfilipi deleted the publicStats branch January 3, 2019 22:23

Ivanidzo4ka reviewed Jan 3, 2019

View reviewed changes

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


		var scoredData = transformer.Transform(dataView);

		var coeffcients = stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100);


		var coeffcients = stats.GetCoefficientStatistics(linearModel, scoredData.Schema["Features"], 100);

		Assert.Equal(19, coeffcients.Length);

making GetCoefficientStatistics public #1979

making GetCoefficientStatistics public #1979

Uh oh!

Conversation

sfilipi commented Dec 28, 2018

Uh oh!

TomFinley Dec 28, 2018 • 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!

TomFinley Dec 28, 2018 • 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!

TomFinley Dec 28, 2018 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi commented Jan 3, 2019

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

sfilipi commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka left a comment

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!

shmoradims left a comment

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!

Ivanidzo4ka Jan 3, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivanidzo4ka Jan 3, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Jan 3, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TomFinley Dec 28, 2018 •

edited

Loading

TomFinley Dec 28, 2018 •

edited

Loading

TomFinley Dec 28, 2018 •

edited by sfilipi

Loading

sfilipi commented Jan 3, 2019 •

edited

Loading

Ivanidzo4ka Jan 3, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Jan 3, 2019 •

edited by sfilipi

Loading

sfilipi Jan 3, 2019 •

edited

Loading