Skip to content

Ova and Pkpd as estimators #865

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 8 commits into from
Sep 14, 2018
Merged

Ova and Pkpd as estimators #865

merged 8 commits into from
Sep 14, 2018

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Sep 7, 2018

Converting OVA and PKPD to derive from TrainerEstimatorBase.
Updating the arguments of those Metalinear learners to have the standard Feature, Label, Weights columns.

@sfilipi sfilipi self-assigned this Sep 7, 2018
@sfilipi sfilipi added the API Issues pertaining the friendly API label Sep 7, 2018
@Zruty0 Zruty0 mentioned this pull request Sep 7, 2018
[TGUI(Label = "Predictor Type", Description = "Type of underlying binary predictor")]
public IComponentFactory<TScalarTrainer> PredictorType;

[Argument(ArgumentType.Multiple, HelpText = "Output calibrator", ShortName = "cali", NullName = "<None>", SignatureType = typeof(SignatureCalibrator))]
[Argument(ArgumentType.Multiple, HelpText = "Output calibrator", ShortName = "cali", SortOrder = 150, NullName = "<None>", SignatureType = typeof(SignatureCalibrator))]
Copy link
Contributor

@TomFinley TomFinley Sep 10, 2018

Choose a reason for hiding this comment

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

150 [](start = 109, length = 3)

What is the purpose of these 150 sort orders? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

i think the old GUI doesn't show them. They are the "advanced" ones?


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

{
public abstract class ArgumentsBase
{
[Argument(ArgumentType.Multiple, HelpText = "Base predictor", ShortName = "p", SortOrder = 1, SignatureType = typeof(SignatureBinaryClassifierTrainer))]
[Argument(ArgumentType.AtMostOnce, HelpText = "Column to use for features", ShortName = "feat", SortOrder = 1)]
public string FeatureColumn = DefaultColumnNames.Features;
Copy link
Contributor

@TomFinley TomFinley Sep 10, 2018

Choose a reason for hiding this comment

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

DefaultColumnNames.Features [](start = 42, length = 27)

DefaultColumnNames.Features [](start = 42, length = 27)

DefaultColumnNames.Features [](start = 42, length = 27)

If this is the design we are going for at least as a temporary measure we ought to have a flag on these to indicate that they should not appear either in entry-points or in the command line/GUI, since they will work contrary to the current mechanisms. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I wonder, what is the purpose of having features and weights passed in for the case of OVA? It seems like something like OVA would take an estimator that decides how to train a model. This internal estimator would potentially know about weights and features, but the external structure seems like it only needs to know about the label so it knows how to "split up" the training.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that a) these should be outside args, and b) they should not be necessary at all, with the exception of Label.


In reply to: 216348132 [](ancestors = 216348132,216346589)

.Append(new MyOva(env, sdcaTrainer))
.Append(new Ova(env, new Ova.Arguments
{
PredictorType = ComponentFactoryUtils.CreateFromFunction(e => sdcaTrainer)
Copy link
Contributor

@Zruty0 Zruty0 Sep 10, 2018

Choose a reason for hiding this comment

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

PredictorType [](start = 24, length = 13)

PredictorType [](start = 24, length = 13)

PredictorType [](start = 24, length = 13)

this is not the right way to do this in the API. I think the right way is to just pass the sdcaTrainer directly to the constructor as a parameter (with an appropriate interface constraint).

Right now you are invoking dependency injection, and we swore we weren't going to do this.

It would of course involve a rework of MetaMulticlassTrainer, but that's pretty much what this work is all about anyway #Closed

Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

🕐

@sfilipi sfilipi changed the title Ova estimator WIP: Ova estimator Sep 11, 2018

namespace Microsoft.ML.Runtime.Learners
{
using TScalarTrainer = ITrainer<IPredictorProducing<Float>>;
using TScalarTrainer = ITrainerEstimator<IPredictionTransformer<IPredictorProducing<float>>, IPredictorProducing<float>>;
Copy link
Member Author

@sfilipi sfilipi Sep 12, 2018

Choose a reason for hiding this comment

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

IPredictionTransformer [](start = 45, length = 22)

should be BinaryPredictionTransformer #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's fine as it is


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

Copy link
Contributor

Choose a reason for hiding this comment

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

Binary is not an interface, so we can't have covariance if we have it there


In reply to: 217234106 [](ancestors = 217234106,217218242)


// the validations in the calibrator check for the feature column, in the RoleMappedData
var trainedData = new RoleMappedData(view, label: trainerLabel, feature: transformer.FeatureColumn);

Copy link
Member Author

@sfilipi sfilipi Sep 12, 2018

Choose a reason for hiding this comment

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

@Zruty0, @[email protected] is there a way to restore all the roles from the trainers? This is only partially correct...

The calibrator training below looks for them.


if (calibratedModel == null)
// calibratedModel = CalibratorUtils.TrainCalibratorIfNeeded(Host, ch, calibrator, _args.MaxCalibrationExamples, trainer, transformer.Model, data) as TScalarPredictor;
calibratedModel = CalibratorUtils.TrainCalibrator(Host, ch, Calibrator, Args.MaxCalibrationExamples, transformer.Model, trainedData) as TScalarPredictor;
Copy link
Member Author

@sfilipi sfilipi Sep 12, 2018

Choose a reason for hiding this comment

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

TrainCalibrator [](start = 54, length = 15)

@Zruty0 should this still be conditional on the trainer, for OVA? #Closed

@@ -57,7 +57,7 @@ public Arguments()
env => new Ova(env, new Ova.Arguments()
{
PredictorType = ComponentFactoryUtils.CreateFromFunction(
e => new FastTreeBinaryClassificationTrainer(e, new FastTreeBinaryClassificationTrainer.Arguments()))
Copy link
Member Author

@sfilipi sfilipi Sep 12, 2018

Choose a reason for hiding this comment

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

e => new FastTreeBinaryClassificationTrain [](start = 26, length = 44)

this is temporary, until FastTree is an estimator. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @sfilipi, honestly I feel like this sort of thing really should not have a default. It seems to be along the same lines as the discussion in #682.


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

@@ -19,6 +17,8 @@
using Microsoft.ML.Runtime.Model;
using Microsoft.ML.Runtime.Model.Pfa;
using Newtonsoft.Json.Linq;
using Microsoft.ML.Runtime.Training;
using System.Collections.Generic;
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

System [](start = 6, length = 6)

sort namespaces #Closed

/// OVA with calibrator
/// </summary>
[Fact]
public void New_OVAWithCalibrator()
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

New_OVAWithCalibrator [](start = 20, length = 21)

move these to a different location: we don't need to expand the set of estimator tests #Closed

@Zruty0
Copy link
Contributor

Zruty0 commented Sep 13, 2018

                .Append(new MyOva(env, sdcaTrainer))

this will not compile, right? #Closed


Refers to: test/Microsoft.ML.Tests/Scenarios/Api/Estimators/Metacomponents.cs:32 in cd05a04. [](commit_id = cd05a04, deletion_comment = False)

/// <param name="maxCalibrationExamples">Number of instances to train the calibrator.</param>
/// <param name="useProbabilities">Use probabilities (vs. raw outputs) to identify top-score category.</param>
public Ova(IHostEnvironment env, TScalarTrainer singleEstimator, string labelColumn = DefaultColumnNames.Label,
bool imputeMissingLabelsAsNegative = false, ICalibratorTrainer calibrator = null,
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

calibrator [](start = 75, length = 10)

note that the old default is not null #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't create the calibrator here, because i need to pass the env..
there is no regression, as i am restoring the Platt calibrator in the base MetaMulticlassTrainer constructor. Is it enough to comment about it in the parameter?


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

Copy link
Contributor

Choose a reason for hiding this comment

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

that's correct


In reply to: 217479852 [](ancestors = 217479852,217233354)

/// OVA with uncalibrated
/// </summary>
[Fact]
public void New_OVAUncalibrated()
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

Uncalibrated [](start = 27, length = 12)

this is still calibrated, I believe, since SDCA binary by default is calibrated #Closed

trainer, predictor, td);
predictor = res as TScalarPredictor;
Host.Check(predictor != null, "Calibrated predictor does not implement the expected interface");
var calibratedModel = transformer.Model as TScalarPredictor;
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

TScalarPredictor [](start = 59, length = 16)

This should be the TDist one #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? The model in the trainer is of type TScalar.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

see the other response


In reply to: 217266436 [](ancestors = 217266436,217233650)

/// Developers should instantiate <see cref="Pkpd"/> by supplying the trainer argument directly to the <see cref="Pkpd"/> constructor
/// using the other public constructor.
/// </summary>
/// <param name="env"></param>
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

param [](start = 13, length = 5)

if they are empty, remove them #Closed

/// Initializes a new instance of the <see cref="Pkpd"/>
/// </summary>
/// <param name="env">The <see cref="IHostEnvironment"/> instance.</param>
/// <param name="singleEstimator">An instance of the <see cref="BinaryPredictionTransformer{IModel}"/> used as the base predictor.</param>
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

singleEstimator [](start = 25, length = 15)

the comment is incorrect. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a trainer, not a transformer


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

// the validations in the calibrator check for the feature column, in the RoleMappedData
var trainedData = new RoleMappedData(view, label: trainerLabel, feature: transformer.FeatureColumn);

var calibratedModel = transformer.Model as TDistPredictor;
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

calibratedModel [](start = 16, length = 15)

this logic is correct, replicate it to OVA, where it is different #Closed

Copy link
Member Author

@sfilipi sfilipi Sep 13, 2018

Choose a reason for hiding this comment

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

@Zruty0 see my other question: the type for the model of the OVA IPredictionTransformer is TScalarPredictor. Double-checkign that it is correct to try casting to a TDistPredictor. Is that because learners that use calibration produce the propabilities and the score, therefore two floats?


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

Copy link
Contributor

Choose a reason for hiding this comment

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

SOME TScalarPredictors are also TDistPredictors, some are not. If the one we have is not, we will need to calibrate. That's the gist of the code in lines 134-136


In reply to: 217477854 [](ancestors = 217477854,217233894)

Host.Check(dist != null, "Calibrated predictor does not implement the expected interface");
Host.Check(dist is IValueMapperDist, "Calibrated predictor does not implement the IValueMapperDist interface");
return dist;
// this should not be necessary when the legacy constructor doesn't exist, and the label colum is not an optional parameter on the
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

colum [](start = 101, length = 5)

typo #Closed

/// Initializes a new instance of <see cref="Ova"/>.
/// </summary>
/// <param name="env">The <see cref="IHostEnvironment"/> instance.</param>
/// <param name="singleEstimator">An instance of the <see cref="BinaryPredictionTransformer{IModel}"/> used as the base predictor.</param>
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

singleEstimator [](start = 25, length = 15)

I would call this 'binary', not 'single' #Closed

@@ -32,19 +32,21 @@
[assembly: EntryPointModule(typeof(OvaPredictor))]
namespace Microsoft.ML.Runtime.Learners
{
using TScalarPredictor = IPredictorProducing<float>;
using TScalarTrainer = ITrainerEstimator<IPredictionTransformer<IPredictorProducing<float>>, IPredictorProducing<float>>;
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

IPredictorProducing [](start = 68, length = 26)

can we change that to TScalarPredictor ?
#Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

can't use TScalarPredictor yet: type could not be found.


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

public bool ImputeMissingLabelsAsNegative;
}

protected readonly TArgs Args;
/// <summary>
/// The label column that the trainer expects. Can be <c>null</c>, which indicates that label
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

Can be null [](start = 55, length = 18)

can it? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the comment since not relevant here; but the LabelColumn can be null, if instantiated through the legacy constructors.
Added a check for it in the new constructors, where it should not be null.


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

Host.CheckValue(inputSchema, nameof(inputSchema));

// Special treatment for label column: we allow different types of labels, so the trainers
// may define their own requirements on the label column.
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

may define their own requirements on the label column [](start = 14, length = 54)

they may not any more #Closed

if (LabelColumn != null)
{
if (!inputSchema.TryFindColumn(LabelColumn.Name, out var labelCol))
throw Host.Except($"Label column '{LabelColumn.Name}' is not found");
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

Except [](start = 31, length = 6)

ExceptSchemaMismatch #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought ExceptSchemaMismatch is when there is a type mismatch. This is checking for existence?


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

Copy link
Contributor

Choose a reason for hiding this comment

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

there's 2 overloads to ExceptSchemaMismatch, one says 'XXX column YYY not found', another says 'XXX column YYY is expected to be ZZZ but is QQQ'


In reply to: 217272278 [](ancestors = 217272278,217234801)

OutputColumns = new[]
{
new SchemaShape.Column(DefaultColumnNames.Score, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false),
new SchemaShape.Column(DefaultColumnNames.PredictedLabel, SchemaShape.Column.VectorKind.Scalar, NumberType.U4, true)
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

true [](start = 127, length = 4)

check what I do with the base class now: you will have to do the same (as in, pass through the keyValues metadata here, if present #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

Skip adding the metadata, since i am creating the LabelColumn on line 77, without metadata?


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

Copy link
Contributor

Choose a reason for hiding this comment

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

you will need metadata for keys. Check what multi-SDCA does


In reply to: 217271448 [](ancestors = 217271448,217235004)

@sfilipi sfilipi changed the title WIP: Ova estimator Ova and Pkpd as estimators Sep 13, 2018
/// <param name="name">The component name.</param>
/// <param name="labelColumn">The label column for the metalinear trainer and the binary trainer.</param>
/// <param name="singleEstimator">The binary estimator.</param>
/// <param name="calibrator">The calibrator. If a calibrator is not explicitely provided, it will default to <see cref="PlattCalibratorCalibratorTrainer"/></param>
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

e [](start = 84, length = 1)

explicitly #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks!


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

Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

:shipit:

/// <summary>
/// OVA with calibrator argument
/// </summary>
[Fact]
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

Fact [](start = 9, length = 4)

I'm sorry, I was not clear on that. Move it to Microsoft.ML.Tests/TrainerEstimators directory. And don't use New_ prefix #Resolved

.Read(new MultiFileSource(dataPath));

var sdcaTrainer = new LinearClassificationTrainer(env, new LinearClassificationTrainer.Arguments { MaxIterations = 100, Shuffle = true, NumThreads = 1 }, "Features", "Label");
var pipeline = new MyConcatTransform(env, "Features", "SepalLength", "SepalWidth", "PetalLength", "PetalWidth")
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

pipeline [](start = 20, length = 8)

call TestEstimatorCore(pipeline, data)

Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

this will run a bunch of tests, including fitting, saving, loading etc.


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

Copy link
Member Author

Choose a reason for hiding this comment

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

couldn't use it because of the MyConcatTransform in the pipeline. It's GetSchemaOuput throws. Will update once the concat estimator is in.


In reply to: 217526641 [](ancestors = 217526641,217526588)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, maybe take it out of the pipeline? :)


In reply to: 217551711 [](ancestors = 217551711,217526641,217526588)

var calibratedModel = transformer.Model as TScalarPredictor;

// restoring the RoleMappedData, as much as we can.
// not having the weight column on the data passed to the TrainCalibrator should be addressed.
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

should be addressed. [](start = 90, length = 20)

write a REVIEW in the beginning, to make that grep-able #Resolved

// Regarding caching, no matter what the internal predictor, we're performing many passes
// simply by virtue of this being a meta-trainer, so we will still cache.
Info = new TrainerInfo(normalization: _trainer.Info.NeedNormalization);

OutputColumns = new[]
Copy link
Contributor

@Zruty0 Zruty0 Sep 13, 2018

Choose a reason for hiding this comment

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

OutputColumns [](start = 12, length = 13)

you no longer need these, right? #Resolved

Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

Thank you @sfilipi

@sfilipi sfilipi merged commit bebf523 into dotnet:master Sep 14, 2018
@sfilipi sfilipi deleted the OVAEstimator branch October 8, 2018 17:18
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 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.

3 participants