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

Calibrator estimators #1816

Open
wants to merge 25 commits into
base: master
from

Conversation

4 participants
@sfilipi
Member

sfilipi commented Dec 4, 2018

Fixes #1622 by creating one CalibratorEstimator per CalibratorTrainer.
Introduces CalibratorTransforms

@sfilipi sfilipi requested review from TomFinley and removed request for TomFinley Dec 4, 2018

@@ -1118,6 +1154,91 @@ public ICalibrator FinishTraining(IChannel ch)
public abstract ICalibrator CreateCalibrator(IChannel ch);
}
public sealed class CalibratorTransformer<TICalibrator> : RowToRowTransformerBase, ISingleFeaturePredictionTransformer<TICalibrator>

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

Add XML docs #Resolved

bool ITransformer.IsRowToRowMapper => true;
public override void Save(ModelSaveContext ctx)

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

Save [](start = 29, length = 4)

add test about save. Add Create method and ctor to load #Resolved

public override Func<int, bool> GetDependencies(Func<int, bool> activeOutput)
=> col => activeOutput(_scoreColIndex);
public override void Save(ModelSaveContext ctx)

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

Save [](start = 33, length = 4)

test, #Resolved

public override Func<int, bool> GetDependencies(Func<int, bool> activeOutput)
=> col => activeOutput(_scoreColIndex);
public override void Save(ModelSaveContext ctx)

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

Save [](start = 33, length = 4)

VersionInfo? #Resolved

/// OVA and calibrators
/// </summary>
[Fact]
public void OVACalibrators()

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

OVACalibrators [](start = 20, length = 14)

Create an API sample for CalibratorEstimator and CalibratorTransformer #Resolved

using System.Collections.Generic;
using System.IO;
using System.Linq;
using Float = System.Single;

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 4, 2018

Member

can you kill it? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 4, 2018

Member

i will on the last iteration, otherwise the PR becomes too noisy..


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

@@ -8,7 +8,6 @@ namespace Microsoft.ML.Runtime.Training
{
public interface ITrainerEstimator<out TTransformer, out TPredictor>: IEstimator<TTransformer>
where TTransformer: ISingleFeaturePredictionTransformer<TPredictor>
where TPredictor: IPredictor

This comment has been minimized.

@TomFinley

TomFinley Dec 4, 2018

Contributor

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

Hmmm. Is removing this necessary?

I guess your goal is to have the calibrator estimator fitting be a trainer estimator, but is that required or helpful, in some fashion? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

Yes, TPredictor is an ICalibrator, for the CalibratorEstimators. ICalibrator is not an IPredictor.


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

This comment has been minimized.

@TomFinley

TomFinley Dec 5, 2018

Contributor

OK. I think that we should then probably then not have this be a training estimator? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

Pete seemed to like the idea. He was ok with removing this constraint.

Reverted the change.


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

This comment has been minimized.

@TomFinley

TomFinley Dec 7, 2018

Contributor

That may be. However I like it less than he does perhaps @sfilipi? But thanks for removing #Resolved

@@ -158,7 +158,7 @@ protected virtual void CheckLabelCompatible(SchemaShape.Column labelCol)
protected virtual RoleMappedData MakeRoles(IDataView data) =>
new RoleMappedData(data, label: LabelColumn?.Name, feature: FeatureColumn.Name, weight: WeightColumn?.Name);
IPredictor ITrainer.Train(TrainContext context) => ((ITrainer<TModel>)this).Train(context);
IPredictor ITrainer.Train(TrainContext context) => (IPredictor)((ITrainer<TModel>)this).Train(context);

This comment has been minimized.

@TomFinley

TomFinley Dec 4, 2018

Contributor

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

Why is this necessary? ITrainer<TModel> where TModel : IPredictor should just be a model without the cast? Is it actually not certain that this is the case? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

Yep, leftover from trying to extend the TrainerEstimatorBase for the CalibratorEstimators. Decided not to go for it, since that extends ITraner, but might make sense to do so.


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

This comment has been minimized.

@TomFinley

TomFinley Dec 5, 2018

Contributor

So I feel like increasing training estimators" to such a large extent basically makes them indistinguishable from general estimators, and I wonder if we should not do that. Is there a cost to having a separate type for training calibrators, while maintaining the integrity of the trainer estimators? #Resolved

sfilipi added some commits Dec 4, 2018

Fixing Score column type.
Splitting the estimators/transformers in a separate file.
Adding calibratorTransformer.
/// OVA and calibrators
/// </summary>
[Fact]
public void FixedPlatCalibratorEstimator()

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

FixedPlatCalibratorEstimator [](start = 20, length = 28)

It generates only 1 and 1 e-15 values. #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

This seems fine now.


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

/// OVA and calibrators
/// </summary>
[Fact]
public void NaiveCalibratorEstimator()

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

NaiveCalibratorEstimator [](start = 20, length = 24)

It generates only 1 and 0 values. #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 5, 2018

Member

This seems fine now.


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

sfilipi added some commits Dec 5, 2018

Fixing the loadable class attribute
Creating one transformer per estimator, so we can save/load
CalibratorTransformer is an abstract base class
Fixing GetDependancies

@sfilipi sfilipi changed the title from WIP: Calibrator estimators to Calibrator estimators Dec 5, 2018

@sfilipi sfilipi self-assigned this Dec 5, 2018

/// [!code-csharp[Calibrators](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Calibrator.cs)]
/// ]]></format>
/// </example>
public abstract class CalibratorEstimatorBase<TCalibratorTrainer, TICalibrator> : IEstimator<CalibratorTransformer<TICalibrator>>

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

CalibratorEstimatorBase [](start = 26, length = 23)

CalibratorTrainerEstimatorBase?
[Update] Nothing. #Closed

var roleMappedData = new RoleMappedData(input, opt: false, roles.ToArray());
using (var ch = Host.Start("Creating calibrator."))
{

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

{ [](start = 8, length = 5)

Redundant "{" and "}". #Resolved

WeightColumn = TrainerUtils.MakeR4ScalarWeightColumn(weightColumn);
}
SchemaShape IEstimator<CalibratorTransformer<TICalibrator>>.GetOutputSchema(SchemaShape inputSchema)

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

Please add a doc string for this function --- For calibrator, the output schema is computed by adding one probability column into input schema. Existing probability column may be overwritten. #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 10, 2018

Member

it won't be overitten. We will have two :)


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

return Create(Host, calibrator);
}
protected abstract CalibratorTransformer<TICalibrator> Create(IHostEnvironment env, TICalibrator calibrator);

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

Why do we need this? Any doc string? #Resolved

/// to the dataset. The Probability column is the value of the score normalized to be a valid probability.
/// </summary>
/// <typeparam name="TICalibrator"></typeparam>
public abstract class CalibratorTransformer<TICalibrator> : RowToRowTransformerBase, ISingleFeaturePredictionTransformer<TICalibrator>

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

If one day, hinge loss is added in FFM, it will not able to use this CalibratorTransformer because of ISingleFeaturePredictionTransformer. A calibrator just maps a scalar to another scalar, I am not sure why ISingleFeaturePredictionTransformer is needed. #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 10, 2018

Member

The Score is considered the Feature column here (one).


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

This comment has been minimized.

@wschin

wschin Dec 11, 2018

Member

Could you please explain this concept in its doc string? Just saying that CalibratorTransform is a trainable model which maps the raw score to probability; that is, score can be viewed as feature in ISingleFeaturePredictionTransformer while probability is treated as the label.


In reply to: 240421876 [](ancestors = 240421876,240339828)

{
_calibrator = calibrator;
_parent = parent;
inputSchema.TryGetColumnIndex(DefaultColumnNames.Score, out _scoreColIndex);

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

TryGetColumnIndex [](start = 28, length = 17)

You might want to replace TryGetColumnIndex with GetColumnOrNull. Please review other places where ISchma-style functions are used (if any). #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 10, 2018

Member

This works better here, because of GetDependenciesCore below needing the index.

Is this on the path to deprecation?


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

This comment has been minimized.

@wschin

wschin Dec 11, 2018

Member

It will be deprecated.


In reply to: 240426878 [](ancestors = 240426878,240340891)

return new[]
{
new Schema.DetachedColumn(DefaultColumnNames.Probability, NumberType.Float, null)
};

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

}; [](start = 15, length = 4)

Not aligned line. #Resolved

@@ -15,7 +15,6 @@ namespace Microsoft.ML.Runtime
/// </summary>
/// <typeparam name="TModel">The <see cref="IPredictor"/> used for the data transformation.</typeparam>
public interface IPredictionTransformer<out TModel> : ITransformer
where TModel : IPredictor
{

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

Why did this get removed? Do you need to modify the doc string to reflect this change? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 10, 2018

Member

got removed because the calibrator transformers derive from here, and their TModel is an ICalibrator.

Thansk for the note on the docs.


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

scoredData = scoredData,
pipeline = pipeline,
transformer = ((TransformerChain<BinaryPredictionTransformer<LinearBinaryPredictor>>)transformer).LastTransformer as BinaryPredictionTransformer<LinearBinaryPredictor>,
};

This comment has been minimized.

@wschin

wschin Dec 10, 2018

Member

Do you wan to test linear regression here? #Resolved

This comment has been minimized.

@sfilipi

sfilipi Dec 10, 2018

Member

no, it would be overkill. As longs as the columns are produced, if the score column is not there, it will faile later with that message.


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

@wschin

wschin approved these changes Dec 12, 2018

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.ML.Data;

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

Do you even sort bro?

labelColumn: "Sentiment",
featureColumn: "Features",
l2Const: 0.001f,
loss: new HingeLoss())); // By specifying loss: new HingeLoss(), StochasticDualCoordinateAscent may train a support vector machine (SVM).

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

may train [](start = 120, length = 9)

is it really may, or will?

/// </summary>
public interface ICalibrator
{
/// <summary> Given a classifier output, produce the probability </summary>

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

[](start = 72, length = 1)

*gasp, no . in the end of sentence?

float PredictProbability(float output);
/// <summary> Get the summary of current calibrator settings </summary>
string GetSummary();

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

I don't see any usages of this either in this repo, or internal one.
Shall we remove it?

We have ICanSaveSummary interface, which i think more suitable than that method.

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

    public static PlattCalibrator Create(IHostEnvironment env, ModelLoadContext ctx)

private?


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1387 in f520da5. [](commit_id = f520da5, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

    public static NaiveCalibrator Create(IHostEnvironment env, ModelLoadContext ctx)

private?


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1032 in f520da5. [](commit_id = f520da5, deletion_comment = False)

internal PavCalibrator(IHostEnvironment env, Float[] mins, Float[] maxes, Float[] values)
internal PavCalibrator(IHostEnvironment env, float[] mins, float[] maxes, float[] values)

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

internal PavCalibrator(IHostEnvironment env, float[] mins, float[] maxes, float[] values) [](start = 8, length = 89)

All other calibrators has exposed public constructor and public properties.
Here you have internal constructor and private readonly _mins, _maxes, _values.

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

    public const string UserName = "Naive Calibrator";

You can still make them internal + bestfriend if that breaks your build.


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


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:909 in e315a4c. [](commit_id = e315a4c, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

public sealed class NaiveCalibratorTrainer : ICalibratorTrainer
Trains Naive bayes calibrator. It explore range of score column and break it into equally sized bins. In each bin probability of belonging to class 1 is the number of class 1 instances in the bin divided by total number of isntances in the bin.

Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:881 in f520da5. [](commit_id = f520da5, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

/// The naive binning-based calibrator

Naive bayes calibrator.
It contain set of bins where in each bin we have probability of result been class 1.
Each bin covers equal range of scores
To determine probability bin we have left border for first bin. Scores which exceed maximum get assign to last bin, Scores which are less than get assign to first bin.


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:973 in f520da5. [](commit_id = f520da5, deletion_comment = False)

private readonly Float[] _binProbs;
private readonly float _binSize;
private readonly float _min;
private readonly float[] _binProbs;

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member

make them public, make _binProbs ImmutableArray.

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

public sealed class PlattCalibratorTrainer : CalibratorTrainerBase
Trains Platt calibrator. If trains logistic regression model on score column.

Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1140 in f520da5. [](commit_id = f520da5, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

    private Double _paramB;

They can be moved to CreateCalibrator code.
They shouldn't be part of class.


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1143 in f520da5. [](commit_id = f520da5, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

public sealed class FixedPlattCalibratorTrainer : ICalibratorTrainer
Creates Platt calibrator based on provided Slope and Offset parameters.

Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1301 in f520da5. [](commit_id = f520da5, deletion_comment = False)

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 12, 2018

    public FixedPlattCalibratorTrainer(IHostEnvironment env, Arguments args)

Make it private/internal. We don't have to expose command line stuff in API.
Make public constructor which will accept slope and offset.


Refers to: src/Microsoft.ML.Data/Prediction/Calibrator.cs:1326 in f520da5. [](commit_id = f520da5, deletion_comment = False)

{
return new PlattCalibrator(_host, _slope, _offset);
}
ICalibrator ICalibratorTrainer.FinishTraining(IChannel ch) => new PlattCalibrator(_host, _slope, _offset);
}
public sealed class PlattCalibrator : ICalibrator, IParameterMixer, ICanSaveModel, ISingleCanSavePfa, ISingleCanSaveOnnx

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 12, 2018

Member
Platt calibrator. Calculate probability following way P(x) = 1 / (1 + exp(-slope * x + offset) (make ref for slope and offset).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment