Skip to content

LightGbm pigstensions #1020

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 5 commits into from
Sep 26, 2018
Merged

LightGbm pigstensions #1020

merged 5 commits into from
Sep 26, 2018

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Sep 25, 2018

Ongoing work to address #754
Those are the LightGbm binary and regression extension methods.

@sfilipi sfilipi added the API Issues pertaining the friendly API label Sep 25, 2018
@sfilipi sfilipi self-assigned this Sep 25, 2018
/// Check that the label, feature, weights is not supplied in the args of the constructor.
/// Those parameters should be internal if they are not used from the maml help code path.
/// </summary>
public static void CheckArgsDefaultColNames(IHost env, string defaultColName, string argValue)
Copy link
Contributor

@TomFinley TomFinley Sep 25, 2018

Choose a reason for hiding this comment

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

IHost env [](start = 52, length = 9)

IExceptionContext please. #Resolved

public static void CheckArgsDefaultColNames(IHost env, string defaultColName, string argValue)
{
if (argValue != defaultColName)
throw env.Except($"Don't supply a value for the {defaultColName} column in the arguments, as it will be ignored. Specify them in the loader, or constructor instead instead.");
Copy link
Contributor

@TomFinley TomFinley Sep 25, 2018

Choose a reason for hiding this comment

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

instead instead [](start = 172, length = 15)

duplicate duplicate #Resolved

@@ -382,6 +382,16 @@ public static SchemaShape.Column MakeR4ScalarWeightColumn(string weightColumn)
return null;
return new SchemaShape.Column(weightColumn, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false);
}

/// <summary>
/// Check that the label, feature, weights is not supplied in the args of the constructor.
Copy link
Contributor

@TomFinley TomFinley Sep 25, 2018

Choose a reason for hiding this comment

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

args [](start = 74, length = 4)

Maybe ought to write out arguments. #Resolved


/// <summary>
/// Check that the label, feature, weights is not supplied in the args of the constructor.
/// Those parameters should be internal if they are not used from the maml help code path.
Copy link
Contributor

@TomFinley TomFinley Sep 25, 2018

Choose a reason for hiding this comment

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

maml help code path [](start = 78, length = 19)

Given the focus we now have I'm a little wary of having language specific to the command line in any new code, that is not somehow directly related to the command line, without plenty of explanatory text. Someone unaware of this software's roots as a tool rather than a library will have absolutely no idea what this means. #Resolved

return rec.Output;
}

private static void CheckUserValues<TVal, TArgs, TPred>(Scalar<TVal> label, Vector<float> features, Scalar<float> weights,
Copy link
Contributor

@Zruty0 Zruty0 Sep 25, 2018

Choose a reason for hiding this comment

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

we are just checking the presence of stuff, right? We could make label a 'PipelineColumn', and the delegates to be just 'Delegate', and this method will not need to be generic #Resolved

{
public static class LightGbmStatics
{
public static Scalar<float> LightGbm(this RegressionContext.RegressionTrainers ctx,
Copy link
Contributor

@Zruty0 Zruty0 Sep 25, 2018

Choose a reason for hiding this comment

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

copy the summary comment here #Resolved

/// <param name="weightColumn">The name for the column containing the initial weight.</param>
/// <param name="advancedSettings">A delegate to apply all the advanced arguments to the algorithm.</param>
/// <param name="learningRate"></param>
Copy link
Contributor

@Zruty0 Zruty0 Sep 25, 2018

Choose a reason for hiding this comment

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

add docs on empty parameters #Resolved

@shauheen shauheen requested a review from codemzs September 25, 2018 19:48
Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thanks @sfilipi !

@sfilipi
Copy link
Member Author

sfilipi commented Sep 25, 2018

close/reopen to trigger a rebuild.

@sfilipi sfilipi closed this Sep 25, 2018
@sfilipi sfilipi reopened this Sep 25, 2018
@sfilipi
Copy link
Member Author

sfilipi commented Sep 26, 2018

close - reopen to trigger rebuild.

@sfilipi sfilipi closed this Sep 26, 2018
@sfilipi sfilipi reopened this Sep 26, 2018
@sfilipi
Copy link
Member Author

sfilipi commented Sep 26, 2018

close/reopen to trigger rebuild

@sfilipi sfilipi closed this Sep 26, 2018
@sfilipi sfilipi reopened this Sep 26, 2018
@shauheen shauheen merged commit a01c80c into dotnet:master Sep 26, 2018
@sfilipi sfilipi deleted the LightGBMXtensions branch October 20, 2018 03:08
@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.

4 participants