Skip to content

Conversion of Hogwild SGD to estimator #1134

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

Merged
merged 7 commits into from
Oct 5, 2018
Merged

Conversation

artidoro
Copy link
Contributor

@artidoro artidoro commented Oct 3, 2018

Ongoing work on converting the trainers to estimators (#754). This PR converts Hogwild SGD (StochasticGradientDescentClassificationTrainer binary classification trainer).

@artidoro artidoro added the API Issues pertaining the friendly API label Oct 3, 2018
Ivanidzo4ka
Ivanidzo4ka previously approved these changes Oct 3, 2018
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@Zruty0 Zruty0 mentioned this pull request Oct 3, 2018
pipe.Append(new StochasticGradientDescentClassificationTrainer(Env, "Features", "Label"));
TestEstimatorCore(pipe, dataView);
Done();
}
Copy link
Member

@sfilipi sfilipi Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, if there is only one test, you can also keep it on the TrainerEstimators file, to have a smaller amount of files. #Resolved

[TlcModule.SweepableDiscreteParam("L2Const", new object[] { 1e-7f, 5e-7f, 1e-6f, 5e-6f, 1e-5f })]
public float L2Const = (float)1e-6;
Copy link
Member

@sfilipi sfilipi Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L2Const [](start = 25, length = 7)

double -check with @TomFinley on this change. Think we tend to keep the names same as the names of the variables in the respective papers. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will do, thank you!


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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked with Tom and he is fine with the name change, as it seems commonly used in the rest of the codebase.


In reply to: 222463578 [](ancestors = 222463578,222460420)

_args.L2Weight = l2Weight;
_args.Check(env);

_loss = _args.LossFunction.CreateComponent(env);
Copy link
Member

@sfilipi sfilipi Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_loss = _a [](start = 10, length = 12)

i think Pete exposed the loss function for SDCA. Might want to do the same. #Resolved

_args.WeightColumn = weightColumn;
_args.MaxIterations = maxIterations;
_args.InitLearningRate = initLearningRate;
_args.L2Weight = l2Weight;
Copy link
Member

@sfilipi sfilipi Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for fast trees, i am printing a warning to the user to let them know that the values of the arguments are being overridden. Since we give them two places to set them, to avoid, feel like we should warn.

   [https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L169](https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L169)

Also:
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L104
#Resolved

@Ivanidzo4ka Ivanidzo4ka dismissed their stale review October 4, 2018 04:21

revoking review

/// be informed about what was learnt.</param>
/// <returns>The predicted output.</returns>
public static (Scalar<float> score, Scalar<float> probability, Scalar<bool> predictedLabel) StochasticGradientDescentClassificationTrainer(this BinaryClassificationContext.BinaryClassificationTrainers ctx,
Scalar<bool> label,
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalar label, [](start = 12, length = 19)

Can you ask Pete or Tom about this one?
I'm just concern what we have Pisgty only on top of bool label. In other universe we used to support R4 where positive label was everything above zero, and negative is everything else and Key value with restriction to key dimensionality is 2.

Bool value is fine, but what if user have Text label, like "Yes" and "No", I don't think we have currently way to transform them to bool, only ToKey, but this method limit us only to bool. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See CheckBinaryLabel method.


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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I will check with them, and let you know.


In reply to: 222534836 [](ancestors = 222534836,222534640)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with Tom and he said that in Pigsty it is important to be very deliberate about the types, and that we will just allow the labels to be Scalar. This is a constraint of the static api.


In reply to: 222775240 [](ancestors = 222775240,222534836,222534640)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

/// <param name="features">The name of the feature column.</param>
/// <param name="weights">The name for the example weight column.</param>
/// <param name="maxIterations">The maximum number of iterations; set to 1 to simulate online learning.</param>
/// <param name="initLearningRate">The initial Learning Rate used by SGD.</param>
Copy link
Member

@sfilipi sfilipi Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learning R [](start = 55, length = 10)

nit: no need for title case #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!


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

Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@artidoro artidoro merged commit d517589 into dotnet:master Oct 5, 2018
@artidoro artidoro deleted the binsgd branch October 5, 2018 21:05
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants