Renaming .Train to .Fit in TrainerEstimators #2528

artidoro · 2019-02-13T07:36:56Z

As further explained in the issue, there were two methods with two different names to train a TrainerEstimator. I renamed one of them so that the additional functionalities come with an overload.

In particular the methods Train(IDataView trainData, IDataView validData), and Train(IDataView trainData, IPredictor initialPredictor) have been renamed to Fit(....

Ivanidzo4ka · 2019-02-13T21:32:18Z

src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineTrainer.cs

@@ -509,7 +513,8 @@ internal static CommonOutputs.BinaryClassificationOutput TrainBinary(IHostEnviro
            return new FieldAwareFactorizationMachinePredictionTransformer(Host, model, trainData.Schema, FeatureColumns.Select(x => x.Name).ToArray());
        }

-        public FieldAwareFactorizationMachinePredictionTransformer Fit(IDataView input) => Train(input);
+        /// <summary> Trains and returns a <see cref="ITransformer"/>.</summary>
+        public FieldAwareFactorizationMachinePredictionTransformer Fit(IDataView input) => Fit(input);


public FieldAwareFactorizationMachinePredictionTransformer Fit(IDataView input) => Fit(input); [](start = 8, length = 94)

public FieldAwareFactorizationMachinePredictionTransformer Fit(IDataView input) => Fit(input); [](start = 8, length = 94)

Ghostbusters theme playing:

If there's something strange,
In your test runs.
Who you gonna blame?
STACKOVERFLOW! #Closed

on a serious note, you call yourself here.
change it to Fit(IDataView input) => Fit(input, null, null)

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

Ah makes sense! Thank you I thought there was something strange thanks a lot 👍 #Closed

Ivanidzo4ka · 2019-02-13T21:35:08Z

src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs

        /// </summary>
        /// <param name="input">The training data set.</param>
-        public MatrixFactorizationPredictionTransformer Fit(IDataView input) => Train(input);
+        public MatrixFactorizationPredictionTransformer Fit(IDataView input) => Fit(input);


Fit(IDataView input) => Fit(input) [](start = 56, length = 34)

Fit(IDataView input) => Fit(input) [](start = 56, length = 34)

See my other comment on your PR. #Closed

Ivanidzo4ka · 2019-02-13T22:55:21Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

+        /// <summary>
+        /// Continues the training of a <see cref="OlsLinearRegressionTrainer"/> using an initial predictor and returns a <see cref="ITransformer"/>.
+        /// </summary>
+        public RegressionPredictionTransformer<OlsLinearRegressionModelParameters> Fit(IDataView trainData, IPredictor initialPredictor)


IPredictor [](start = 108, length = 10)

Can we file issue, to clean IPredictor from public interface? @TomFinley trying to hide them in #2251 and we should accept strictly typed classes, like LinearModelParameters here. #Closed

#2553

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

Ivanidzo4ka

codecov · 2019-02-14T00:12:29Z

Codecov Report

Merging #2528 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
+ Coverage   71.26%   71.27%   +0.01%     
==========================================
  Files         797      799       +2     
  Lines      141292   141377      +85     
  Branches    16118    16118              
==========================================
+ Hits       100692   100769      +77     
- Misses      36138    36147       +9     
+ Partials     4462     4461       -1

Flag	Coverage Δ
#Debug	`71.27% <100%> (+0.01%)`	⬆️
#production	`67.6% <100%> (+0.01%)`	⬆️
#test	`85.36% <100%> (ø)`	⬆️

codecov · 2019-02-14T00:13:41Z

Codecov Report

Merging #2528 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2528      +/-   ##
==========================================
- Coverage   71.43%   71.42%   -0.01%     
==========================================
  Files         801      801              
  Lines      141784   141793       +9     
  Branches    16135    16135              
==========================================
- Hits       101278   101277       -1     
- Misses      36039    36048       +9     
- Partials     4467     4468       +1

Flag	Coverage Δ
#Debug	`71.42% <100%> (-0.01%)`	⬇️
#production	`67.71% <100%> (-0.01%)`	⬇️
#test	`85.51% <100%> (ø)`	⬆️

wschin · 2019-02-14T19:00:25Z

src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs

@@ -64,6 +64,7 @@ public abstract class TrainerEstimatorBase<TTransformer, TModel> : ITrainerEstim
            WeightColumn = weight;
        }

+        /// <summary> Trains and returns a <see cref="ITransformer"/>.</summary>


///
Trains and returns a .
[](start = 8, length = 72)

Please mention something like Derived class can overload this function. For example, in <see cref="FastTreeBinaryClassificationTrainer">, another <see cref="FastTreeBinaryClassificationTrainer.Fit"> which consumes two data sets, training and validation sets, is implemented.

Why? This is a base class, anyone who derives from it can overload everything.

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

This is for design documentation. Closely related things should be documented explicitly. We are not only writing code for Microsoft, ML.NET is for the public.

In reply to: 256974019 [](ancestors = 256974019,256967851)

It's weird to have references to derived classes in base class especially it it's not functionality of base class. References from top to bottom make sense since you rely on base class and you know what is underneath you. Over way around, I don't see how it make sense.

In reply to: 256976207 [](ancestors = 256976207,256974019,256967851)

src/Microsoft.ML.FastTree/FastTreeRegression.cs

wschin · 2019-02-14T19:03:01Z

src/Microsoft.ML.FastTree/FastTreeRanking.cs

@@ -456,7 +456,10 @@ protected override string GetTestGraphHeader()
        protected override RankingPredictionTransformer<FastTreeRankingModelParameters> MakeTransformer(FastTreeRankingModelParameters model, Schema trainSchema)
        => new RankingPredictionTransformer<FastTreeRankingModelParameters>(Host, model, trainSchema, FeatureColumn.Name);

-        public RankingPredictionTransformer<FastTreeRankingModelParameters> Train(IDataView trainData, IDataView validationData = null)
+        /// <summary>
+        /// Trains a <see cref="FastTreeRankingTrainer"/> using both training and validation data, returns a <see cref="ITransformer"/>.


ITransformer [](start = 120, length = 12)

Whenever possible, I'd choose a more specific type. Does RankingPredictionTransformer<FastTreeRankingModelParameters> look good to you?

wschin · 2019-02-14T19:04:20Z

src/Microsoft.ML.FastTree/FastTreeClassification.cs

@@ -277,7 +277,10 @@ protected override void InitializeTests()
            CalibratedModelParametersBase<FastTreeBinaryModelParameters, PlattCalibrator> model, Schema trainSchema)
            => new BinaryPredictionTransformer<CalibratedModelParametersBase<FastTreeBinaryModelParameters, PlattCalibrator>>(Host, model, trainSchema, FeatureColumn.Name);

-        public BinaryPredictionTransformer<CalibratedModelParametersBase<FastTreeBinaryModelParameters, PlattCalibrator>> Train(IDataView trainData, IDataView validationData = null)
+        /// <summary>
+        /// Trains a <see cref="FastTreeBinaryClassificationTrainer"/> using both training and validation data, returns a <see cref="ITransformer"/>.


ITransformer [](start = 133, length = 12)

As mentioned in other comments, it'd be more informative to specify the exact returned type here. Could you please also double-check other uses of ITransformer in those doc strings.

Yes will search everywhere

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

wschin

Overall LGTM so approved. Please do address those comments around doc strings before merging. Other questions about design can be closed by creating issues if you feel they are really problems.

artidoro added the API Issues pertaining the friendly API label Feb 13, 2019

artidoro self-assigned this Feb 13, 2019

artidoro requested review from Ivanidzo4ka, wschin, TomFinley and yaeldekel February 13, 2019 07:36

artidoro force-pushed the trainfit branch from 2a53e07 to 6970bc1 Compare February 13, 2019 18:48

Ivanidzo4ka reviewed Feb 13, 2019

View reviewed changes

artidoro force-pushed the trainfit branch from 6970bc1 to ed5d171 Compare February 13, 2019 22:22

Ivanidzo4ka reviewed Feb 13, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 13, 2019

View reviewed changes

wschin reviewed Feb 14, 2019

View reviewed changes

src/Microsoft.ML.FastTree/FastTreeRegression.cs Outdated Show resolved Hide resolved

wschin reviewed Feb 14, 2019

View reviewed changes

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs Show resolved Hide resolved

wschin approved these changes Feb 14, 2019

View reviewed changes

artidoro force-pushed the trainfit branch from ddf29a8 to b6ff064 Compare February 15, 2019 00:22

artidoro added 4 commits February 14, 2019 16:29

renamed Train and added xml doc

9452d98

merge fix

5a43a83

fixing stackoverflow

9107908

made comment more precise when possible

73b4739

artidoro force-pushed the trainfit branch from b6ff064 to 73b4739 Compare February 15, 2019 00:42

added comment as wei-sheng suggested

0ff69a2

artidoro merged commit b2587fa into dotnet:master Feb 15, 2019

artidoro deleted the trainfit branch March 13, 2019 17:55

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming .Train to .Fit in TrainerEstimators #2528

Renaming .Train to .Fit in TrainerEstimators #2528

artidoro commented Feb 13, 2019

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

artidoro Feb 13, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

Ivanidzo4ka Feb 14, 2019

Ivanidzo4ka left a comment

codecov bot commented Feb 14, 2019

codecov bot commented Feb 14, 2019 •

edited

Loading

wschin Feb 14, 2019 •

edited

Loading

Ivanidzo4ka Feb 14, 2019

wschin Feb 14, 2019 •

edited

Loading

Ivanidzo4ka Feb 14, 2019

wschin Feb 14, 2019

wschin Feb 14, 2019

artidoro Feb 14, 2019

wschin left a comment

Renaming .Train to .Fit in TrainerEstimators #2528

Renaming .Train to .Fit in TrainerEstimators #2528

Conversation

artidoro commented Feb 13, 2019

Ivanidzo4ka Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

artidoro Feb 13, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 14, 2019

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2019

Codecov Report

codecov bot commented Feb 14, 2019 • edited Loading

Codecov Report

wschin Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 14, 2019

Choose a reason for hiding this comment

wschin Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 14, 2019

Choose a reason for hiding this comment

wschin Feb 14, 2019

Choose a reason for hiding this comment

wschin Feb 14, 2019

Choose a reason for hiding this comment

artidoro Feb 14, 2019

Choose a reason for hiding this comment

wschin left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

artidoro Feb 13, 2019 •

edited by Ivanidzo4ka

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

Ivanidzo4ka Feb 13, 2019 •

edited

Loading

codecov bot commented Feb 14, 2019 •

edited

Loading

wschin Feb 14, 2019 •

edited

Loading

wschin Feb 14, 2019 •

edited

Loading