Skip to content

Refactoring of Constructors #2100

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

Open
abgoswam opened this issue Jan 10, 2019 · 2 comments
Open

Refactoring of Constructors #2100

abgoswam opened this issue Jan 10, 2019 · 2 comments
Labels
P2 Priority of the issue for triage purpose: Needs to be fixed at some point.

Comments

@abgoswam
Copy link
Member

abgoswam commented Jan 10, 2019

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

Example:

A.

public FastTreeRankingTrainer(IHostEnvironment env,
string labelColumn = DefaultColumnNames.Label,
string featureColumn = DefaultColumnNames.Features,
string groupIdColumn = DefaultColumnNames.GroupId,
string weightColumn = null,
int numLeaves = Defaults.NumLeaves,
int numTrees = Defaults.NumTrees,
int minDatapointsInLeaves = Defaults.MinDocumentsInLeaves,
double learningRate = Defaults.LearningRates,
Action<Arguments> advancedSettings = null)

B.

internal FastTreeRankingTrainer(IHostEnvironment env, Arguments args)
: base(env, args, TrainerUtils.MakeR4ScalarColumn(args.LabelColumn))
{

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
Copy link
Contributor

TomFinley commented Jan 10, 2019

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

@abgoswam
Copy link
Member Author

abgoswam commented Jan 10, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Priority of the issue for triage purpose: Needs to be fixed at some point.
Projects
None yet
Development

No branches or pull requests

3 participants