Skip to content

WIP Introduce I*PredictionKind*TrainerFactory and propagate them #670

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

Closed

Conversation

Ivanidzo4ka
Copy link
Contributor

Fixes #669.

Need to kill all LoadableClasses attributes, and figure out why command line parser throw assert.

@eerhardt this is related to comment in your PR regarding subcomponents, and I just don't want you to do same job since this is about 90% complete.

@@ -24,7 +24,7 @@ public abstract class ArgumentsBase
{
[Argument(ArgumentType.Multiple, HelpText = "Base predictor", ShortName = "p", SortOrder = 1, SignatureType = typeof(SignatureBinaryClassifierTrainer))]
[TGUI(Label = "Predictor Type", Description = "Type of underlying binary predictor")]
public IComponentFactory<TScalarTrainer> PredictorType;
Copy link
Member

Choose a reason for hiding this comment

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

See the comment here #622 (comment) why I didn't do it this way to begin with. I don't think we should be closing off this interface to a sub-interface, because it limits what kinds of classes can implement the interface.

return Args.PredictorType != null ?
Args.PredictorType.CreateComponent(Host) :
new LinearSvm(Host, new LinearSvm.Arguments());
return Args.PredictorType.CreateComponent(Host);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if someone sets Args.PredictorType to null?

@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2018

I see we are propagating the pattern the Arguments class also being a factory which creates the component.
I had a discussion with @Zruty0 about this pattern. Here are the reasons I don't like it:

  1. It breaks the https://en.wikipedia.org/wiki/Single_responsibility_principle. Is the Arguments class the data holder for the information used to instantiate the component? Or is it the factory that creates the component? In this case, it is both.
  2. It feels like a "trick" in the API. And it looks awkward in code:
var ovaArgs = new Ova.Arguments();
ovaArgs.PredictorType = new FastTreeBinaryClassificationTrainer.Arguments();

var ova = new Ova(env, ovaArgs);

IMO it is unintuitive to set PredictorType to an instance of an Arguments class (which is supposed to be the data holder of the information to pass into the FastTreeBinaryClassificationTrainer component).

@ericstj
Copy link
Member

ericstj commented Aug 9, 2018

I also find the Arugments behaving as a factory confusing. Factories should take arguments, not be arguments. If something is going to produce types it shouldn't be called arguments, its actually a "configured" factory. @eerhardt can you formalize the alternative so that we can see the difference?

@Ivanidzo4ka Ivanidzo4ka closed this Aug 9, 2018
@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2018

My proposed alternative is 2 fold:

  1. With my first change, I introduced the concept of a SimpleComponentFactory, which just wraps a delegate to create the component and is capable of creating any component. Example:
    var ovaArgs = new Ova.Arguments();
    ovaArgs.PredictorType = new SimpleComponentFactory<ITrainer<IPredictorProducing<float>>>(
        (e) => new FastTreeBinaryClassificationTrainer(e, new FastTreeBinaryClassificationTrainer.Arguments()));

    var ova = new Ova(env, ovaArgs);

This is a bit verbose, so in the places where we want a stream-lined API, we could have (2).

  1. We could introduce concrete "convenience" factory classes. So in this case where I need a BinaryClassifier:
    var ovaArgs = new Ova.Arguments();
    ovaArgs.PredictorType = new BinaryClassifierFactory(
        (e) => new FastTreeBinaryClassificationTrainer(e, new FastTreeBinaryClassificationTrainer.Arguments()));

    var ova = new Ova(env, ovaArgs);

Or even if we really want to stream-line it, have a factory for the specific component:

    var ovaArgs = new Ova.Arguments();
    ovaArgs.PredictorType = new FastTreeBinaryClassificationFactory(new FastTreeBinaryClassificationTrainer.Arguments());

    var ova = new Ova(env, ovaArgs);

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants