Skip to content
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

Configurable Threshold for binary models #2969

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Ivanidzo4ka
Copy link
Member

Ivanidzo4ka commented Mar 14, 2019

fixes #2465

Ivanidzo4ka added some commits Mar 14, 2019

@Ivanidzo4ka Ivanidzo4ka requested review from rogancarr , TomFinley and sfilipi Mar 14, 2019

return new TransformerChain<BinaryPredictionTransformer<TModel>>(transformers.ToArray());
}

public BinaryPredictionTransformer<TModel> ChangeModelThreshold<TModel>(BinaryPredictionTransformer<TModel> model, float threshold)

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 14, 2019

Author Member

needs documentation

public BinaryPredictionTransformer<TModel> ChangeModelThreshold<TModel>(BinaryPredictionTransformer<TModel> model, float threshold)
where TModel : class
{
if (model.Threshold == threshold)

This comment has been minimized.

@sfilipi

sfilipi Mar 15, 2019

Member

if (model.Threshold == threshold) [](start = 12, length = 33)

do you want to warn here?

This comment has been minimized.

@TomFinley

TomFinley Mar 22, 2019

Contributor

I think we should provide the same warning that C# does when you have a variable like int a = 5 and then assign 5 to it later.


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

if (chain.LastTransformer.Threshold == threshold)
return chain;
List<ITransformer> transformers = new List<ITransformer>();
var predictionTransformer = chain.LastTransformer;

This comment has been minimized.

@sfilipi

sfilipi Mar 15, 2019

Member

chain.LastTransformer [](start = 40, length = 21)

I don't like the assumption that the predictor is the last one, it might not be.

IMO the only API existing for this should be the second one.

If we have to have this API, i think we should minimally take in the index of the predicitonTransformer, in the pipeline, and check whether that transformer is a binaryTransformer.

This comment has been minimized.

@TomFinley

TomFinley Mar 22, 2019

Contributor

That's a good point @sfilipi. I think you're probably right about this.


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

{
}

class Answer

This comment has been minimized.

@sfilipi

sfilipi Mar 15, 2019

Member

Answer [](start = 14, length = 6)

Prediction or DataWithPrediction

//predictor.Threshold = 0.01; // Not possible
var mlContext = new MLContext(seed: 1);

var data = mlContext.Data.LoadFromTextFile<TweetSentiment>(GetDataPath(TestDatasets.Sentiment.trainFilename),

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

Can we try not to load file everywhere? It will be faster to just use in-memory data.

This comment has been minimized.

@rogancarr

rogancarr Mar 15, 2019

Contributor

We have standard test datasets saved to files that we use in tests.

/// <param name="chain">Chain of transformers.</param>
/// <param name="threshold">New threshold.</param>
/// <returns></returns>
public TransformerChain<BinaryPredictionTransformer<TModel>> ChangeModelThreshold<TModel>(TransformerChain<BinaryPredictionTransformer<TModel>> chain, float threshold)

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member
Suggested change
public TransformerChain<BinaryPredictionTransformer<TModel>> ChangeModelThreshold<TModel>(TransformerChain<BinaryPredictionTransformer<TModel>> chain, float threshold)
public TransformerChain<BinaryPredictionTransformer<TModel>> ChangeDecisionThreshold<TModel>(TransformerChain<BinaryPredictionTransformer<TModel>> chain, float threshold)

Maybe?

/// <param name="chain">Chain of transformers.</param>
/// <param name="threshold">New threshold.</param>
/// <returns></returns>
public TransformerChain<BinaryPredictionTransformer<TModel>> ChangeModelThreshold<TModel>(TransformerChain<BinaryPredictionTransformer<TModel>> chain, float threshold)

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

I am not sure if this should be a new function. Could we add a parameter, threshold, to all binary trainers? #Pending

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Mar 15, 2019

Author Member

Ok, we can add that as parameter to binary trainer. Question is if you train your model, how you gonna change threshold? Retrain model?
I think this method has right to live.


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

This comment has been minimized.

@wschin

wschin Mar 15, 2019

Member

Retrain looks fine to me. I really don't feel adding a helper function is a good idea. This is not a Transformer, so I expect it will become a orphan in the future. Like FFM, PFI and so on don't care about it because it's not a standard binary classifier.


In reply to: 266088129 [](ancestors = 266088129,266062138)

This comment has been minimized.

@TomFinley

TomFinley Mar 22, 2019

Contributor

I am not sure if this should be a new function. Could we add a parameter, threshold, to all binary trainers? #Pending

Historically we have found that adding options to "all" trainers just invites inconsistency and is a nightmare from a maintainability perspective. For those reasons we no longer do that. So I strongly object to that. There is also the larger, more practical problem that choosing the right threshold is something that you can only really do once you have investigated it -- that is, it is very often a post training operation, not something you do pre-training.

This sort of "composable" nature of IDataView is actually I think something we need to reiterate, since it was the key to making our development efforts scale; and that composability is built around having simple, comprehensible units of computation. Not big bundled components that tried to do everything themselves. We already tried that way, and life was a lot worse and more inconsistent before we had it, and reverting to the "old ways" of every conceivable functionality bundled into a single operation would just reintroduce the old problems that led us to move to many operations of simple operators in the first place.

This comment has been minimized.

@TomFinley

TomFinley Mar 22, 2019

Contributor

As I think about it more, there's something about this idea of getting ITransformer implementors from existing ITransformer implementors that I find very appealing. Not just for this (which is a worthy use of this idea), but many other scenarios as well.

So for example, certain regressor algorithms are parametric w.r.t. their labels (in fact, most are). But there's a problem with merely normalizing the label, because then the predicted label is according to that same scale. In sklearn you could accomplish this fairly easily via the inverse_transform method on their equivalent of what we call a normalizer, the StandardScalar. So imagine you could get from a NormalizerTransformer another NormalizerTransformer that provides the inverse offset and scaling for any affine normalization, and whatnot. That would be pretty nice, would it not be?

So far from discouraging this pattern, I think we should do more of it.

var predictionTransformer = chain.LastTransformer;
foreach (var transform in chain)
{
if (transform != predictionTransformer)

This comment has been minimized.

@TomFinley

TomFinley Mar 22, 2019

Contributor

predictionTransformer [](start = 33, length = 21)

Can we change this just a little please? I would prefer that we just add all transforms except the last unconditionally, which would be a fairly easy thing to do.

Edit: Actually no @sfilipi is right, I think operating over chains is misguided now that I see her argument...

@TomFinley

This comment has been minimized.

Copy link
Contributor

TomFinley commented Mar 22, 2019

Helper function for IEstimator<...>?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.