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

Modify API for advanced settings (GAM learners) #2199

Merged
merged 2 commits into from Jan 22, 2019

Conversation

abgoswam
Copy link
Member

Towards #1798 .

This PR addresses the following algos

  • BinaryClassificationGamTrainer
  • RegressionGamTrainer

The following changes have been made:

  1. Two public extension methods, one for simple arguments and the other for advanced options
  2. Make constructors internal
  3. Pass Options objects as arguments instead of Action delegate
  4. Rename Arguments to Options
  5. Rename Options objects as options (instead of args or advancedSettings used so far)

args.Check(Host);
_options = args;
options.Check(Host);
_options = options;
Copy link
Member

@sfilipi sfilipi Jan 22, 2019

Choose a reason for hiding this comment

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

_options = options; [](start = 10, length = 21)

check #Resolved

Copy link
Member

@sfilipi sfilipi 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

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹

@@ -50,8 +50,8 @@ public sealed class Arguments : ArgumentsBase
/// <summary>
/// Initializes a new instance of <see cref="BinaryClassificationGamTrainer"/>
/// </summary>
internal BinaryClassificationGamTrainer(IHostEnvironment env, Arguments args)
: base(env, args, LoadNameValue, TrainerUtils.MakeBoolScalarLabel(args.LabelColumn))
internal BinaryClassificationGamTrainer(IHostEnvironment env, Options options)
Copy link
Contributor

@rogancarr rogancarr Jan 22, 2019

Choose a reason for hiding this comment

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

One thing that I've noticed about this switch to options is that we have two APIs for each learner: One with options, and one with a subset of the options spelled out. Wouldn't it make sense to only have one constructor that took options, and have the Catalog interface handle the multiplicity of ways to construct it?

That is, only have

new MyTrainer(env, options)

And then have the catalog do this:

public static BinaryClassificationGamTrainer GeneralizedAdditiveModels(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,
            string labelColumn = DefaultColumnNames.Label,
            string featureColumn = DefaultColumnNames.Features,
            string weights = null,
            int numIterations = GamDefaults.NumIterations,
            double learningRate = GamDefaults.LearningRates,
            int maxBins = GamDefaults.MaxBins)
        {
            Contracts.CheckValue(catalog, nameof(catalog));
            var env = CatalogUtils.GetEnvironment(catalog);
            var options = new BinaryClassificationGamTrainer.Options{
                xyz = etc
            }
            return new BinaryClassificationGamTrainer(env, options);
        }
``` #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.

Indeed. That's issue #2100

Recall, we had issues with the constructor unification for FastTreeRanking.

P0 is to get the public APIs ready.
#2100 is P1/P2 in that context.


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

@abgoswam abgoswam merged commit 3bc01c4 into dotnet:master Jan 22, 2019
@abgoswam abgoswam deleted the abgoswam/action_arguments_gam branch February 20, 2019 16:59
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 25, 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.

None yet

3 participants