Uh oh!
There was an error while loading. Please reload this page.
Conversion of prior and random trainers to estimators - #876
Conversation
| private static readonly TrainerInfo _info = new TrainerInfo(normalization: false, caching: false); | ||
| public override TrainerInfo Info => _info; | ||
| public RandomTrainer(IHostEnvironment env, Arguments args) |
There was a problem hiding this comment.
RandomTrainer [](start = 15, length = 13)
You should not remove this constructor. Once you start writing the test you will discover that it is necessary.
Add a second constructor instead. Both here and for Prior #Closed
| /// </summary> | ||
| public sealed class RandomTrainer : TrainerBase<RandomPredictor> | ||
| public sealed class RandomTrainer : TrainerEstimatorBase<BinaryPredictionTransformer<RandomPredictor>, RandomPredictor> |
There was a problem hiding this comment.
TrainerEstimatorBase [](start = 40, length = 20)
It doesn't help you to derive from this base class, and it also hurts: you now have to take label, feature and weight columns, but you don't actually need them at all.
So, don't derive from TrainerEstimatorBase and just implement ITrainerEstimator #Closed
oh look, a Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:99 in dda393a. [](commit_id = dda393a, deletion_comment = False) |
| public RandomTrainer(IHostEnvironment env, Arguments args) | ||
| : base(env, LoadNameValue) | ||
| protected override SchemaShape.Column[] OutputColumns => throw new NotImplementedException(); | ||
There was a problem hiding this comment.
That is obviously not sufficient. You need to list the columns that you are going to output. #Closed
There was a problem hiding this comment.
| RoleMappedData trainRoles = new RoleMappedData(cachedTrain); | ||
| var pred = Train(new TrainContext(trainRoles)); | ||
| return new BinaryPredictionTransformer<RandomPredictor>(Host, pred, cachedTrain.Schema, trainRoles.Schema.Feature.Name); |
There was a problem hiding this comment.
trainRoles.Schema.Feature.Name [](start = 100, length = 30)
this will make features a required column, which we don't want #Closed
There was a problem hiding this comment.
Let's follow through and formally say that feature column is also optional in PredictionTransformerBase: right now it is not
In reply to: 216845418 [](ancestors = 216845418)
| return new RandomPredictor(Host, Host.Rand.Next()); | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
summary [](start = 13, length = 7)
this is not a good comment.
A good comment has to describe the purpose of the method, and not its implementation.
In this particular case, I think, it's not necessary to have a summary comment at all, given that you're just implementing the interface, and the method is commented there. #Closed
Uh oh!
There was an error while loading. Please reload this page.
Still waiting for tests. #Closed |
| public void BinaryClassifierPerceptronTest() | ||
| { | ||
| var binaryPredictors = new[] { TestLearners.perceptron}; | ||
| var binaryPredictors = new[] { TestLearners.perceptron}; |
There was a problem hiding this comment.
on}; [](start = 64, length = 4)
undo this change #Closed
| ***** Unexpected failure. Please refer to https://aka.ms/MLNetIssue to file an issue with details ***** | ||
| ***** Error log has been saved to '%Temp%\%ErrorLog%', please refer to https://aka.ms/MLNetIssue to file an issue with details ***** | ||
| ===== Begin detailed dump ===== | ||
| (1) Unexpected exception: Object reference not set to an instance of an object., 'System.NullReferenceException' |
There was a problem hiding this comment.
Unexpected exception [](start = 4, length = 20)
this is not a correct behavior #Closed
| ***** Unexpected failure. Please refer to https://aka.ms/MLNetIssue to file an issue with details ***** | ||
| ***** Error log has been saved to '%Temp%\%ErrorLog%', please refer to https://aka.ms/MLNetIssue to file an issue with details ***** | ||
| ===== Begin detailed dump ===== | ||
| (1) Unexpected exception: Could not find file '%Output% 'System.IO.FileNotFoundException' |
There was a problem hiding this comment.
Unexpected exception [](start = 3, length = 21)
make sure your code doesn't throw exceptions #Closed
| private readonly int _seed; | ||
| private readonly object _instanceLock; | ||
| private readonly Random _random; | ||
| private readonly TauswortheHybrid _random; |
There was a problem hiding this comment.
TauswortheHybrid [](start = 25, length = 16)
would IRandom suffice here? #Closed
| public TModel Model { get; } | ||
| public PredictionTransformerBase(IHost host, TModel model, ISchema trainSchema, string featureColumn) | ||
| public PredictionTransformerBase(IHost host, TModel model, ISchema trainSchema, string featureColumn = null) |
There was a problem hiding this comment.
= null [](start = 108, length = 7)
nope, that should still be required param. Just 'null' should now be acceptable. #Closed
| Model = model; | ||
| FeatureColumn = featureColumn; | ||
| if (!trainSchema.TryGetColumnIndex(featureColumn, out int col)) | ||
| if (!trainSchema.TryGetColumnIndex(featureColumn, out int col) && (featureColumn != null)) |
There was a problem hiding this comment.
&& [](start = 75, length = 2)
swap the 2 conditions or you get a wrong error :) #Closed
| using Microsoft.ML.Runtime.Training; | ||
| using Microsoft.ML.Runtime.Internal.Internallearn; | ||
| using Microsoft.ML.Core.Data; | ||
| using System.Linq; |
There was a problem hiding this comment.
System [](start = 6, length = 6)
sort namespaces #Closed
Zruty0
commented
Sep 13, 2018
Now waiting for test containing In reply to: 420450005 [](ancestors = 420450005) |
| @@ -211,12 +236,16 @@ public sealed class Arguments | |||
| public override TrainerInfo Info => _info; | |||
| public PriorTrainer(IHostEnvironment env, Arguments args) | |||
There was a problem hiding this comment.
public [](start = 8, length = 6)
private #Closed
There was a problem hiding this comment.
| public PriorTrainer(IHostEnvironment env, Arguments args) | ||
| : base(env, LoadNameValue) | ||
| : base(Contracts.CheckRef(env, nameof(env)).Register(LoadNameValue), MakeFeatureColumn(DefaultColumnNames.Features), MakeLabelColumn(DefaultColumnNames.Label), null) | ||
| { |
There was a problem hiding this comment.
{ [](start = 8, length = 1)
keep the args check. #Resolved
Same question as below: Why should this be private? Isn't this the only constructor of RandomTrainer? In reply to: 422096175 [](ancestors = 422096175) Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:58 in 3dcb705. [](commit_id = 3dcb705, deletion_comment = False) |
Zruty0
commented
Sep 17, 2018
you should actually have a parameterless constructor, and this one needs to be present just for the component catalog, and it can be made private now. In reply to: 422106631 [](ancestors = 422106631,422096175) Refers to: src/Microsoft.ML.StandardLearners/Standard/Simple/SimpleTrainers.cs:58 in 3dcb705. [](commit_id = 3dcb705, deletion_comment = False) |
| [Fact] | ||
| public void TestEstimatorPrior() | ||
| { | ||
| using (var env = new TlcEnvironment()) |
There was a problem hiding this comment.
env [](start = 23, length = 3)
Use baseclass Env, no need to create another one. #Resolved
| { | ||
| } | ||
| public PriorTrainer(IHost host, SchemaShape.Column feature, SchemaShape.Column label, SchemaShape.Column weight) |
There was a problem hiding this comment.
public [](start = 8, length = 6)
Can it be private?
I understand what you use TrainerEstimatorBase which requires feature column, but for this class it's pointless, and exposing it to user would be potentially confusing. #Resolved
| Float lab = default(Float); | ||
| Float weight = 1; | ||
| var getWeight = colWeight >= 0 ? cursor.GetGetter<float>(colWeight) : null; | ||
| float lab = default(float); |
There was a problem hiding this comment.
float [](start = 36, length = 5)
you can omit it. #Resolved
| /// </summary> | ||
| /// <param name="host"></param> | ||
| /// <param name="feature"></param> | ||
| /// <param name="label"></param> |
There was a problem hiding this comment.
either omit, or put something into them #Resolved
| } | ||
| } | ||
| // Learns the prior distribution for 0/1 class labels and just outputs that. |
There was a problem hiding this comment.
// [](start = 4, length = 2)
Make it ///
| /// values for the column names that does not appear in <paramref name="data"/>'s schema will result in an exception being thrown, | ||
| /// but if <c>true</c> such values will be ignored</param> | ||
| public RoleMappedData(IDataView data, string label, string feature, | ||
| public RoleMappedData(IDataView data, string label, string feature = null, |
There was a problem hiding this comment.
= null [](start = 74, length = 7)
undo this #Resolved
There was a problem hiding this comment.
I will handle this in my code then! Sorry about that
In reply to: 218244002 [](ancestors = 218244002)
| /// <summary> | ||
| /// Initializes PriorTrainer object. | ||
| /// </summary> | ||
| public PriorTrainer(IHost host, SchemaShape.Column label, SchemaShape.Column weight) |
There was a problem hiding this comment.
SchemaShape.Column label [](start = 40, length = 24)
The constructor signature should bePriorTrainer(IHostEnvironment env, string labelColumn, string weightColumn = null) #Resolved
There was a problem hiding this comment.
Ok, I made those Strings instead of SchemShape.Column.
Thank you!
In reply to: 218244513 [](ancestors = 218244513)
| private static SchemaShape.Column MakeLabelColumn(string labelColumn) | ||
| => new SchemaShape.Column(labelColumn, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false); | ||
There was a problem hiding this comment.
don't think you're using this #Resolved
| var pipe = new RandomTrainer(Env); | ||
| // Test only that the schema propagation works. | ||
| // REVIEW: the save/load is not preserving the full state of the random predictor. This is unfortunate, but we don't care too much at this point. |
There was a problem hiding this comment.
Is it still the case?
If yes, can you write down what "full" state are we missing?
There was a problem hiding this comment.
This comment was written by Pete. I don't know exactly what "full state" refers to. I am going to search more in depth to understand what is causing the problem and file a separate issue if it is needed. The behavior that we observed is that the data produced by the model that was saved + loaded back is different from the data produced by the original model.
In reply to: 218520614 [](ancestors = 218520614)
Fixes#875.
Converted prior and random trainers to estimators.
Allowed feature column to be null for both prior and random estimator.