Skip to content

OneVsAllClassifier fits two trainers for binary classification task #3933

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

Open
rtanase opened this issue Jun 28, 2019 · 6 comments
Open

OneVsAllClassifier fits two trainers for binary classification task #3933

rtanase opened this issue Jun 28, 2019 · 6 comments
Labels
enhancement New feature or request P2 Priority of the issue for triage purpose: Needs to be fixed at some point.

Comments

@rtanase
Copy link

rtanase commented Jun 28, 2019

System information

  • OS version/distro:
  • .NET Version (eg., dotnet --info):

Issue

  • What did you do?
  • What happened?
  • What did you expect?
    I expected OneVsAll to identify there are only two classes and train a single learner.

Source code / logs

Internally the OneVersusAllTrainer will always instantiate as many binary classifiers as the number of classes. This is inefficient for binary classification, as just a single trainer is needed. I understand that I can just use an out of the box binary classification for this, but it will simplify the usage.

@wschin
Copy link
Member

wschin commented Jul 1, 2019

So you're saying that if the number of classes is 2, OneVersusAllTrainer should only produce one binary classifier?

@rtanase
Copy link
Author

rtanase commented Jul 1, 2019

yeah, I don't think it should be necessary to train 2 binary classifiers, one for each of the classes, when a single one would do the trick. Right?

@wschin
Copy link
Member

wschin commented Jul 1, 2019

From the perspective you pointed out, yes. On the other hand, the model trained by OVA is a list of model, if we follow your suggestion ML.NET will end up with

  • OVA with 2 classes --> output model is OutputModel = new Model().
  • OVA with > 2 classes --> output model is OutputModel = new Model[number of classes].

It looks a bit wired because the index i in OutputModel[i] means differently in the two cases above. In other words, it's not a desired strongly typed way to ML.NET's public APIs.

Inspired by your comment, if there are only two classes, ML.NET can internally train a binary classifier and copy it to OutputModel[0] and OutputModel[1].

@rtanase
Copy link
Author

rtanase commented Jul 1, 2019

As long as there shouldn't be 2 trainings involved, then it should be fine. The public API should remain same. I wanna avoid having to go through the dataset to figure out how many classes are there to choose between a straight BinaryClassifier or a OneVsRest (which will again go through the dataset to determine number of classes).

If going with training a single model and storing it within OutputModel[0] and OutputModel[1], can those 2 models be exactly the same or does one need to be inverse of the first one?

@wschin
Copy link
Member

wschin commented Jul 1, 2019

The later one should have negative weights of the first one, I assume. For example, if OutputModel[0]'s weight vector is [1, 2, 3], OutputModel[1]'s weight vector should be [-1, -2, -3]. This PR will expose the trained models to users so that we can check if that assumption is true. If that assumption is true, I'd expect a small change to support your idea.

@wschin wschin added the enhancement New feature or request label Jul 2, 2019
@wschin
Copy link
Member

wschin commented Jul 2, 2019

Now I feel doing one binary classification is better than copying the weights.

@yaeldekel yaeldekel added the P2 Priority of the issue for triage purpose: Needs to be fixed at some point. label Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2 Priority of the issue for triage purpose: Needs to be fixed at some point.
Projects
None yet
Development

No branches or pull requests

3 participants