Uh oh!
There was an error while loading. Please reload this page.
Fixing names of trainer estimators - #2903
Conversation
Codecov Report
@@ Coverage Diff @@## master #2903 +/- ##
==========================================
- Coverage 71.82% 71.82% -0.01%
==========================================
Files 812 812 Lines 142719 142719 Branches 16092 16092 ==========================================
- Hits 102513 102510 -3 - Misses 35827 35830 +3
Partials 4379 4379
|
| var data = mlContext.Data.LoadFromEnumerable(samples); | ||
| var options = new ML.Trainers.RandomizedPrincipalComponentAnalyzer.Options() | ||
| var options = new ML.Trainers.RandomizedPcaAnomalyDetectionTrainer.Options() |
There was a problem hiding this comment.
RandomizedPcaAnomalyDetectionTrainer [](start = 42, length = 36)
can we leave "trainer" out of the name? #Resolved
There was a problem hiding this comment.
used acronym for Pca. Kept suffix Trainer in the name of the class
In reply to: 264101278 [](ancestors = 264101278)
| // Define the trainer options. | ||
| var options = new AveragedPerceptronTrainer.Options() | ||
| var options = new AveragedPerceptronBinaryClassificationTrainer.Options() |
There was a problem hiding this comment.
AveragedPerceptronBinaryClassificationTrainer [](start = 30, length = 45)
I vote for AveragePerceptron, since in this case, there aren't one for each task. #Resolved
| var advancedPipeline = mlContext.Transforms.Text.FeaturizeText("SentimentText", "Features") | ||
| .Append(mlContext.BinaryClassification.Trainers.StochasticDualCoordinateAscent( | ||
| new SdcaBinaryTrainer.Options { | ||
| new StochasticDualCoordinateAscentBinaryClassificationTrainer.Options { |
There was a problem hiding this comment.
StochasticDualCoordinateAscentBinaryClassificationTrainer [](start = 42, length = 57)
StochasticDualCoordinateAscentBinaryClassification #Resolved
| .Append(ml.Clustering.Trainers.KMeans( | ||
| new KMeansPlusPlusTrainer.Options | ||
| .Append(ml.Clustering.Trainers.KMeansPlusPlus( | ||
| new KMeansPlusPlusClusteringTrainer.Options |
There was a problem hiding this comment.
KMeansPlusPlusClusteringTrainer [](start = 24, length = 31)
just KMeansPlusPlus maybe #Resolved
| private readonly float[] _labels; | ||
| public ObjectiveImpl(Dataset trainData, RegressionGamTrainer.Options options) : | ||
| public ObjectiveImpl(Dataset trainData, GeneralizedAdditiveModelRegressionTrainer.Options options) : |
There was a problem hiding this comment.
GeneralizedAdditiveModelRegressionTrainer [](start = 52, length = 41)
GeneralizedAdditiveModelRegression #Resolved
| string outputColumnName = "Features"; | ||
| var pipeline = ml.Transforms.Concatenate(outputColumnName, new[] { "Age", "Parity", "Induced" }) | ||
| .Append(ml.Clustering.Trainers.KMeans(outputColumnName, numberOfClusters: 2)); | ||
| .Append(ml.Clustering.Trainers.KMeansPlusPlus(outputColumnName, numberOfClusters: 2)); |
There was a problem hiding this comment.
| .Append(ml.Clustering.Trainers.KMeansPlusPlus(outputColumnName,numberOfClusters:2)); | |
| .Append(ml.Clustering.Trainers.KMeans(outputColumnName,numberOfClusters:2)); | |
| ``` #Resolved |
There was a problem hiding this comment.
The algorithm that is actually implemented is KmeansPlusPlus . The underlying class is also called KMeansPlusPlus
This is also discussed here : #2762 (comment) #Resolved
There was a problem hiding this comment.
The algorithm that is actually implemented is KmeansPlusPlus . The underlying class is also called KMeansPlusPlus
This is also discussed here : #2762 (comment)
In reply to: 264484798 [](ancestors = 264484798)
There is also |
artidoro
commented
Mar 12, 2019
This is will solve part of #2623. |
Tanks for pointing this out. This should be called In reply to: 471806679 [](ancestors = 471806679) |
| // estimator, as opposed to a regular trainer. | ||
| var trainerEstimator = new MulticlassLogisticRegression(env, LabelColumnName, FeatureColumnName); | ||
| return TrainerUtils.MapTrainerEstimatorToTrainer<MulticlassLogisticRegression, | ||
| var trainerEstimator = new LogisticRegressionMulticlassClassificationTrainer(env, LabelColumnName, FeatureColumnName); |
There was a problem hiding this comment.
LogisticRegressionMulticlassClassificationTrainer [](start = 55, length = 49)
this will get ppl confused because it has both Regression and Multiclass on the name, but can't think of a good way to deal with it. would it be ok to just call it Logit @wschin@TomFinley
There was a problem hiding this comment.
I would especially hesitate to call it Logit . Logits has a different interpretation related to the unnormalized log-probabilities
In reply to: 264516270 [](ancestors = 264516270)
| @@ -1,4 +1,4 @@ | |||
| MulticlassLogisticRegression bias and non-zero weights | |||
| LogisticRegressionMulticlassClassificationTrainer bias and non-zero weights | |||
There was a problem hiding this comment.
LogisticRegressionMulticlassClassificationTrainer [](start = 0, length = 49)
how can this be shorter... anyone in favor of dropping 'Classification'
| (env, labelName, featuresName, weightsName) => | ||
| { | ||
| var trainer = new SdcaBinaryTrainer(env, labelName, featuresName, weightsName, l2Regularization, l1Threshold, numberOfIterations); | ||
| var trainer = new SdcaCalibratedBinaryClassificationTrainer(env, labelName, featuresName, weightsName, l2Regularization, l1Threshold, numberOfIterations); |
There was a problem hiding this comment.
SdcaCalibratedBinaryClassificationTrainer [](start = 38, length = 41)
Did leave a note below, but i'd be ok dropping Classification from all BinaryClassification and MulticlassClassification.
There was a problem hiding this comment.
note below .. am not getting you ? The changes here are as per what we summarized in #2762 (comment)
In reply to: 264518672 [](ancestors = 264518672)
| /// <summary> | ||
| /// Predict a target using generalized additive models trained with the <see cref="BinaryClassificationGamTrainer"/>. | ||
| /// Predict a target using generalized additive models trained with the <see cref="GamBinaryClassificationTrainer"/>. |
There was a problem hiding this comment.
I wonder if we should connect the acronyms in the doc:
Predict a target using generalized additive models (GAM) trained with the <see cref="GamBinaryClassificationTrainer"/>. #Resolved
There was a problem hiding this comment.
Thanks for the suggestion. Will fix. #Resolved
| /// ]]></format> | ||
| /// </example> | ||
| public static KMeansPlusPlusTrainer KMeans(this ClusteringCatalog.ClusteringTrainers catalog, | ||
| public static KMeansPlusPlusTrainer KMeansPlusPlus(this ClusteringCatalog.ClusteringTrainers catalog, |
There was a problem hiding this comment.
I think we should be consistent everywhere about this name. If we really intend for this to be KMeansPlusPlus, then we should update all the places that just use KMeans:
The name of the above class:public static class KMeansClusteringExtensions
The name of the assembly:Microsoft.ML.KMeansClustering
Is there any confusion about just using the name KMeans? Are there other KMeans algorithms besides KMeans++? It feels like we should be fine just using KMeans, but I'll leave it up to the experts. #Resolved
There was a problem hiding this comment.
Kmeans++ is a slightly modified version of Kmeans that has some smarts for choosing the initial cluster centers. The trainer estimator we have currently implements Kmeans++
For the specific trainer implementation we have currently, we are using KMeansPlusPlus for the MLContext name and KMeansPlusPlusTrainer the Class name
I can envision that in the future one might even want to implement the vanilla Kmeans algorithm itself. As such to me it makes sense to keep the name of the static class and assembly with just KMeans in the prefix.
In reply to: 264707009 [](ancestors = 264707009)
There was a problem hiding this comment.
My feeling is that no one will add Kmeans as long as Kmeans++ exists. As you mentioned, Kmeans++ is a member of Kmeans family. Why can't we call it Kmeans? If someone want to implement the original Kmeans, it can be called NaiveKmeans. #Resolved
There was a problem hiding this comment.
Sure. That makes sense. Will rename it to KMeans to keep things uniform #Resolved
There was a problem hiding this comment.
Thanks. I also noticed there is a property InitializationAlgorithm to specify the initialization mechanism
So yeah. It should be just KMeans
In reply to: 264770186 [](ancestors = 264770186)
There was a problem hiding this comment.
| using TScalarTrainer = ITrainerEstimator<ISingleFeaturePredictionTransformer<IPredictorProducing<float>>, IPredictorProducing<float>>; | ||
| public abstract class MetaMulticlassTrainer<TTransformer, TModel> : ITrainerEstimator<TTransformer, TModel>, ITrainer<IPredictor> | ||
| public abstract class MetaTrainer<TTransformer, TModel> : ITrainerEstimator<TTransformer, TModel>, ITrainer<IPredictor> |
There was a problem hiding this comment.
Here's a special case of our rule:
{TypeOfTask} is added only only when the algorithm supports multiple kinds of tasks
I don't believe MetaTrainer is a good name here since it too general. And technically, Meta isn't an algorithm. That name makes it sound like it can train anything. I think we should keep "multiclass" in the name. So probably MetaMulticlassClassificationTrainer. #Closed
There was a problem hiding this comment.
Yeah. It makes sense to treat this as a special case. #Resolved
| { | ||
| /// <summary> | ||
| /// Predict a target using a linear classification model trained with <see cref="SgdBinaryTrainer"/>. | ||
| /// Predict a target using a linear classification model trained with <see cref="SgdCalibratedTrainer"/>. |
There was a problem hiding this comment.
Here's a place where the acronym Sgd isn't expanded out in the summary comments. #Resolved
| .AppendCacheCheckpoint(mlContext) | ||
| // Add the SDCA regression trainer. | ||
| .Append(mlContext.Regression.Trainers.StochasticDualCoordinateAscent(labelColumnName: "Target", featureColumnName: "FeatureVector")); | ||
| .Append(mlContext.Regression.Trainers.Sdca(labelColumnName: "Target", featureColumnName: "FeatureVector")); |
There was a problem hiding this comment.
when we update the cookbook samples files, it usually means the cookbook needs to be updated. #Resolved
There was a problem hiding this comment.
abgoswam
commented
Mar 12, 2019
Thanks folks for the review comments! |
Fixes#2762 and #2172