-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixing parameters in ML.NET Public API #2665
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ public static class KMeansClusteringExtensions | |
/// </summary> | ||
/// <param name="catalog">The clustering catalog trainer object.</param> | ||
/// <param name="featureColumn">The features, or independent variables.</param> | ||
/// <param name="weights">The optional example weights.</param> | ||
/// <param name="exampleWeightColumnName">The name of the example weight column (optional).</param> | ||
/// <param name="clustersCount">The number of clusters to use for KMeans.</param> | ||
/// <example> | ||
/// <format type="text/markdown"> | ||
|
@@ -29,7 +29,7 @@ public static class KMeansClusteringExtensions | |
/// </example> | ||
public static KMeansPlusPlusTrainer KMeans(this ClusteringCatalog.ClusteringTrainers catalog, | ||
string featureColumn = DefaultColumnNames.Features, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So we're calling this In the original issue you proposed calling this Is this on someone's radar? If we are not going to fix it (we should!) a second best alternative would be to be at least locally consistent. Not sure what @rogancarr and @eerhardt and @sfilipi think? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue #2177 proposes we use suffix ColumnName consistently In this PR I am making that change for weight, groupid parameters. In a follow up PR I will do it for label and features. So once both PRs go in, we will mark #2177 as complete, and the following convention will be used
This is certainly on my radar, as we drive towards consistency in the APIs . I have also updated the description of the issue so it has reference to issue #2177 . Hope that sounds good ? #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very good, thank you @abgoswam ! Sorry missed that discussion. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomFinley. I have consolidated all the changes in this PR. So this PR addresses {features, labels, groupid, weight} column names in a consistent manner. In reply to: 258779314 [](ancestors = 258779314) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TomFinley. I have consolidated all the changes in this PR. So this PR addresses {features, labels, groupid, weight} column names in a consistent manner. In reply to: 258776812 [](ancestors = 258776812) |
||
string weights = null, | ||
string exampleWeightColumnName = null, | ||
int clustersCount = KMeansPlusPlusTrainer.Defaults.ClustersCount) | ||
{ | ||
Contracts.CheckValue(catalog, nameof(catalog)); | ||
|
@@ -38,7 +38,7 @@ public static KMeansPlusPlusTrainer KMeans(this ClusteringCatalog.ClusteringTrai | |
var options = new KMeansPlusPlusTrainer.Options | ||
{ | ||
FeatureColumn = featureColumn, | ||
WeightColumn = weights != null ? Optional<string>.Explicit(weights) : Optional<string>.Implicit(DefaultColumnNames.Weight), | ||
WeightColumn = exampleWeightColumnName != null ? Optional<string>.Explicit(exampleWeightColumnName) : Optional<string>.Implicit(DefaultColumnNames.Weight), | ||
ClustersCount = clustersCount | ||
}; | ||
return new KMeansPlusPlusTrainer(env, options); | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh. Am I wrong or is there literally no way to have non-weighted training if you happen to have a column named
Weight
????It looks like there's an existing issue #1065 that is capturing this. Not your code, so don't have to fix, I just notice this way. #Resolved
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I see it's incidentally assigned to you @abgoswam . 😄 Maybe after this PR we can think about how to address this. #Resolved
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We should fix this behavior as part of #1065 (in a separate PR). #Closed