search

dotnet / machinelearning

ML.NET is an open source and cross-platform machine learning framework for .NET.
https://dot.net/ml
MIT License
8.94k stars 1.86k forks source link

Refactoring of Constructors #2100

Open abgoswam opened 5 years ago

abgoswam commented 5 years ago

Currently in our codebase, we have two constructors that are used for initialization of the underlying object.

Example:

A. https://github.com/dotnet/machinelearning/blob/bdc9a9e1348abfdd2287d43e2633089e9eba36c7/src/Microsoft.ML.FastTree/FastTreeRanking.cs#L75-L84

B. https://github.com/dotnet/machinelearning/blob/bdc9a9e1348abfdd2287d43e2633089e9eba36c7/src/Microsoft.ML.FastTree/FastTreeRanking.cs#L93-L95

We need to bringing the public API surface to the desired shape. As such, we are making both constructors internal, and fixing other issues with the public API as outlined in #1798.

Additionally, constructor (B) has all the details for constructing the underlying object. As such, we can delete constructor (A) altogether.

NOTE: We will do this issue only after finishing the work to bring public API to the desired shape .

Constructors that need closer look towards unification:

SdcaBinaryTrainer SdcaMultiClassTrainer SdcaRegressionTrainer StochasticGradientDescentClassificationTrainer FastTreeRankingTrainer FastTreeRegressionTrainer FastTreeBinaryClassificationTrainer FastForestClassification FastForestRegression PoissonRegression LogisticRegression BinaryClassificationGamTrainer LightGbmBinaryTrainer LightGbmMulticlassTrainer LightGbmRankingTrainer LightGbmRegressorTrainer

@TomFinley @glebuk @shauheen @sfilipi @artidoro @rogancarr

TomFinley commented 5 years ago

Good, so #1098 will be "the way" unambiguously for the public API, I guess, for v1.

abgoswam commented 5 years ago

Yeap. That is why as part of the work for #1798 we are making the constructors internal.