Public API for remaining learners #1901

najeeb-kazmi · 2018-12-18T04:08:10Z

Predictors covered in this PR:

EnsemblePredictorBase
EnsembleDistributionPredictor
EnsemblePredictor
EnsembleMultiClassPredictor
GamPredictorBase
BinaryClassificationGamPredictor
RegressionGamPredictor
PcaPredictor
FieldAwareFactorizationmachinePredictor
MultiClassLogisticRegressionPredictor`
MultiClassNaiveBayesPredictor
OvaPredictor
PkpdPredictor
RandomPredictor
PriorPredictor

Ivanidzo4ka · 2018-12-18T06:12:49Z

/// The ctor takes an alignment value, which must be a power of two at least sizeof(Float).

would be nice if you update comments as well. #Resolved

Refers to: src/Microsoft.ML.CpuMath/AlignedArray.cs:16 in d3ea4df. [](commit_id = d3ea4df, deletion_comment = False)

src/Microsoft.ML.FastTree/GamRegression.cs

Ivanidzo4ka · 2018-12-18T06:28:24Z

    internal abstract void CheckLabel(RoleMappedData data);

Curios, why it has to be internal? Did someone actually call it outside of class? #Resolved

Refers to: src/Microsoft.ML.FastTree/GamTrainer.cs:245 in d3ea4df. [](commit_id = d3ea4df, deletion_comment = False)

src/Microsoft.ML.PCA/PcaTrainer.cs

src/Microsoft.ML.StandardLearners/AssemblyInfo.cs

Ivanidzo4ka · 2018-12-18T06:40:44Z

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

+            _latentWeightsAligned.CopyFrom(latentWeightsAligned.AsSpan());
+        }
+
+        internal FieldAwareFactorizationMachineModelParameters(IHostEnvironment env, bool norm, int fieldCount, int featureCount, int latentDim,
            float[] linearWeights, AlignedArray latentWeightsAligned) : base(env, LoaderSignature)


float[] linearWeights, AlignedArray latentWeightsAligned [](start = 12, length = 56)

@wschin, are this two similar array with only difference is one of them is array and second is align array?
If yes, I don't understand why I need to pass both of them, and not just get first and construct align version.
If not, why they called this way?

linearWeights is linear coefficient where SSE is not very helpful to compute related values. In contrast, latentWeights is the latent vector of each feature and heavily uses SEE.

This is all nice from internal point of view.
My question are user specific.
Does user need to provide this TWO SAME ARRAYS or not?
If not, can you formulate rule according to which we can construct align array?

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

Same array? LinearWeights and latentWeights are two independent things.

I shouldn't review code at 11 pm, you are right.
Still I don't think we should expect user to pass align array and construct it internally.

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

In the new iteration, I have added methods to return latent weights and linear weights to the user. The latent dimension is something specified by the user at training. The latent weights are converted to the aligned weights by adjusting the latent dimension. This is now hidden from the user. The expectation is that user will call FieldAwareFactorizationMachines.GetLatentWeights() which will have the unadjusted latent dimension, then they can adjust the weights as they want, and pass it to the public constructor. The latent dimension will be adjusted during construction.

In reply to: 242693412 [](ancestors = 242693412,242666580)

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

Ivanidzo4ka · 2018-12-18T06:44:28Z

src/Microsoft.ML.FastTree/GamClassification.cs

        public override PredictionKind PredictionKind => PredictionKind.BinaryClassification;

-        public BinaryClassificationGamPredictor(IHostEnvironment env, int inputLength, Dataset trainset,
+        public GamBinaryClassificationModelParameters(IHostEnvironment env, int inputLength, Dataset trainset,


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

Do you plan to add comments to all public constructors in separate PR? #Resolved

Separate PR sometimes means forever. #Resolved

I'm making the GAM constructors internal because they use Dataset object, which is in Microsoft.ML.FastTree.Internal that I don't think end user of API should be expected to know. I have filed issue #1948 to document it.

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

src/Microsoft.ML.Ensemble/Trainer/EnsembleDistributionModelParameters.cs

src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParameters.cs

src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParametersBase.cs

src/Microsoft.ML.FastTree/FastTree.cs

src/Microsoft.ML.KMeansClustering/KMeansModelParameters.cs

sfilipi

🕐

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

src/Microsoft.ML.CpuMath/AlignedArray.cs

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs

revoking review

najeeb-kazmi · 2018-12-20T22:33:32Z

    internal abstract void CheckLabel(RoleMappedData data);

Yeah, it's overridden in GamRegression and GamVlassification, but those are derived classes so I'll make it private protected.

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

Refers to: src/Microsoft.ML.FastTree/GamTrainer.cs:245 in d3ea4df. [](commit_id = d3ea4df, deletion_comment = False)

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

TomFinley

Ooof, quite a lot of work I see @najeeb-kazmi ! Thanks for fixing the names in so many places, and making so much more internal.

wschin · 2018-12-21T16:52:30Z

Overall looks good. Two comments:
(1) Do you have at least a test for each public function?
(2) For vector and matrix arguments, do you specify their shapes and the meaning of the i-th (or (i,j)-element for matrix where i is row index and j is column index) element?

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

sfilipi · 2018-12-21T23:49:49Z

src/Microsoft.ML.CpuMath/AlignedArray.cs

+            Items.AsSpan().CopyTo(dst);
+        }
+
+        public void CopyTo(Span<float> dst, int index, int count)
        {
            Contracts.Assert(0 <= count && count <= _size);
            Contracts.Assert(dst != null);


double-checking that this is intentional?

Must be. I did not touch this code, except Float -> float!

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

sfilipi · 2018-12-21T23:51:33Z

src/Microsoft.ML.FastTree/GamRegression.cs

@@ -107,44 +107,44 @@ protected override SchemaShape.Column[] GetOutputColumnsCore(SchemaShape inputSc
        }


unrelated to your PR, but since i stumped upon this, is this used at all?

I guess it is part of TrainerEstimatorBase and has to be implemented

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

sfilipi · 2018-12-21T23:53:20Z

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs

+                            _latentWeightsAligned[indexAligned + k] = 0;
+                    }
+                }
+            }


Tagging @wschin to check on this.

sfilipi

najeeb-kazmi requested review from TomFinley, Zruty0, wschin and sfilipi December 18, 2018 04:08

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.FastTree/GamRegression.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.PCA/PcaTrainer.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.PCA/PcaTrainer.cs Show resolved Hide resolved

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.StandardLearners/AssemblyInfo.cs Show resolved Hide resolved

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

Ivanidzo4ka reviewed Dec 18, 2018

View reviewed changes

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.Ensemble/Trainer/EnsembleDistributionModelParameters.cs Show resolved Hide resolved

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParameters.cs Show resolved Hide resolved

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParameters.cs Show resolved Hide resolved

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.Ensemble/Trainer/EnsembleModelParametersBase.cs Show resolved Hide resolved

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.FastTree/FastTree.cs Show resolved Hide resolved

sfilipi reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.KMeansClustering/KMeansModelParameters.cs Show resolved Hide resolved

sfilipi previously requested changes Dec 18, 2018

View reviewed changes

sfilipi reviewed Dec 18, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.CpuMath/AlignedArray.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 18, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 18, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Show resolved Hide resolved

wschin reviewed Dec 18, 2018

View reviewed changes

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs Outdated Show resolved Hide resolved

Zruty0 assigned najeeb-kazmi Dec 18, 2018

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

najeeb-kazmi added 2 commits December 20, 2018 19:04

REducing public surface of remaining predictors

391de01

Addressing comments, adding doc strings

8fd2064

najeeb-kazmi force-pushed the 1703 branch from 831ed40 to 8fd2064 Compare December 21, 2018 03:04

TomFinley approved these changes Dec 21, 2018

View reviewed changes

wschin reviewed Dec 21, 2018

View reviewed changes

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Outdated Show resolved Hide resolved

wschin reviewed Dec 21, 2018

View reviewed changes

...ft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineModelParameters.cs Show resolved Hide resolved

sfilipi reviewed Dec 21, 2018

View reviewed changes

Adressing comments, adding sample

8ccf3d4

sfilipi reviewed Dec 21, 2018

View reviewed changes

sfilipi approved these changes Dec 21, 2018

View reviewed changes

najeeb-kazmi merged commit 00577c0 into dotnet:master Dec 22, 2018

TomFinley mentioned this pull request Dec 23, 2018

Make FastTree/LightGBM learned model suitable for public consumption #1960

Closed

najeeb-kazmi deleted the 1703 branch January 30, 2020 01:13

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

		@@ -107,44 +107,44 @@ protected override SchemaShape.Column[] GetOutputColumnsCore(SchemaShape inputSc
		}

Public API for remaining learners #1901

Public API for remaining learners #1901

Uh oh!

Conversation

najeeb-kazmi commented Dec 18, 2018

Uh oh!

Ivanidzo4ka commented Dec 18, 2018 • edited by najeeb-kazmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Ivanidzo4ka commented Dec 18, 2018 • edited by najeeb-kazmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Ivanidzo4ka Dec 18, 2018 • edited by najeeb-kazmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Dec 18, 2018 • edited by najeeb-kazmi 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfilipi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

najeeb-kazmi commented Dec 20, 2018

Uh oh!

Uh oh!

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

wschin commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Ivanidzo4ka commented Dec 18, 2018 •

edited by najeeb-kazmi

Loading

Ivanidzo4ka commented Dec 18, 2018 •

edited by najeeb-kazmi

Loading

Ivanidzo4ka Dec 18, 2018 •

edited by najeeb-kazmi

Loading

wschin Dec 18, 2018 •

edited by najeeb-kazmi

Loading

wschin commented Dec 21, 2018 •

edited

Loading