New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public API for Tree predictors #1837

merged 6 commits into from Dec 11, 2018


None yet
4 participants

najeeb-kazmi commented Dec 6, 2018

Fix #1701

Internalized and explicitly implemented the following interfaces implemented by FastTreePredictionWrapper:

  • ICanSaveInIniFormat
  • ICanSaveInSourceCode
  • ICanSaveSummary
  • ICanSaveSummaryInKeyValuePairs
  • ICanGetSummaryAsIRow
  • IFeatureContributionMapper
  • IQuantileValueMapper
  • IQuantileRegressionPredictor
  • IValueMapperDist

Renamed FastTreePredictionWrapper to TreeEnsembleModelParameters and descendants to XYZModelParameters. Reduced public surface of TreeEnsembleModelParameters and descendants.

Added public constructors for TreeEnsembleModelParameters and descendants.

Added a sample showing FastTreeRegressionModelParameters operations.

Internalize and explicitly implement ICanSAveInIniFormat, ICanSaveInS…
…ourceCode, ICanSaveSummary, ICanSaveSummaryInKeyValuePairs, and ICanGetSummaryAsIRow


najeeb-kazmi added some commits Dec 7, 2018

Internalize and explicitly implement IFeatureContributionMapper, IQua…
…ntileValueMapper, IQuantileRegressionPredictor. Rename FastTreePredictionWrapper to TreeEnsembleModelParameters and all descendants to XyzModelParameters

@najeeb-kazmi najeeb-kazmi changed the title from [WIP] Public API for Tree predictors to Public API for Tree predictors Dec 11, 2018

@@ -467,12 +467,12 @@ private static VersionInfo GetVersionInfo()
protected override uint VerCategoricalSplitSerialized => 0x00010005;
internal FastTreeRegressionPredictor(IHostEnvironment env, TreeEnsemble trainedEnsemble, int featureCount, string innerArgs)

This comment has been minimized.


sfilipi Dec 11, 2018


internal [](start = 8, length = 8)

i still think those should be internal. There is nothing else that can create the predictor/model params besides training..
Only the class needs to be public IMO.. but I won't block on it.. you can take it with Tom/Pete and potentially change on a later PR.

This comment has been minimized.


najeeb-kazmi Dec 11, 2018


Makes sense. I'll keep these constructors public for now.


This comment has been minimized.


wschin commented Dec 11, 2018

    public const string LoadNameValue = "FastTreeBinaryClassification";

Can this be internal or even more conservative? #Resolved

Refers to: src/Microsoft.ML.FastTree/FastTreeClassification.cs:111 in a5aa3b9. [](commit_id = a5aa3b9, deletion_comment = False)


This comment has been minimized.


wschin commented Dec 11, 2018

    public const string LoaderSignature = "RandomPredictor";

internal? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:114 in a5aa3b9. [](commit_id = a5aa3b9, deletion_comment = False)


wschin approved these changes Dec 11, 2018 edited

LGTM. Just some minor comments but please still address them before merging.

// Get the leaf and the leaf value for a row of data with Parity = 1, Induced = 1 in the first tree.
var testRow = new VBuffer<float>(2, new[] { 1.0f, 1.0f });
List<int> path = default;
var leaf = modelParams.GetLeaf(0, in testRow, ref path);

This comment has been minimized.


Ivanidzo4ka Dec 11, 2018


var leaf = modelParams.GetLeaf(0, in testRow, ref path); [](start = 12, length = 56)

How many leaves you have in this tree?
If it's only few, can we actually read all leaf values, and also their relationship and drew small tree in comments?


(node 0, value: 1.1)
|---->(leaf 0, value: 2.4)
|---> (leaf 1,value: 3.5)

@najeeb-kazmi najeeb-kazmi merged commit 25abf91 into dotnet:master Dec 11, 2018

2 checks passed

MachineLearning-CI #20181211.9 succeeded
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment