In the public surface area, all occurrences of Argument keyword replaced with Options #2563

abgoswam · 2019-02-15T01:08:27Z

For public API surface, all occurrences of Argument keyword replaced with Options

Several internal classes continue to be called Arguments. We can target those post V1.0 since they are not part of public surface area anyway.

Ivanidzo4ka · 2019-02-15T01:27:05Z

src/Microsoft.ML.FastTree/FastTree.cs

@@ -49,7 +49,7 @@ internal static class FastTreeShared
    public abstract class FastTreeTrainerBase<TArgs, TTransformer, TModel> :
        TrainerEstimatorBaseWithGroupId<TTransformer, TModel>
        where TTransformer : ISingleFeaturePredictionTransformer<TModel>
-        where TArgs : TreeArgs, new()
+        where TArgs : TreeOptions, new()


TArgs [](start = 14, length = 5)

TOptions #Resolved

Ivanidzo4ka · 2019-02-15T01:27:24Z

src/Microsoft.ML.FastTree/FastTree.cs

@@ -49,7 +49,7 @@ internal static class FastTreeShared
    public abstract class FastTreeTrainerBase<TArgs, TTransformer, TModel> :
        TrainerEstimatorBaseWithGroupId<TTransformer, TModel>
        where TTransformer : ISingleFeaturePredictionTransformer<TModel>
-        where TArgs : TreeArgs, new()
+        where TArgs : TreeOptions, new()
        where TModel : class
    {
        protected readonly TArgs Args;


TArgs Args [](start = 27, length = 10)

TOptions Options; #Resolved

Options cause compile errors because of conflict with class Options. Renamed to FastTreeTrainerOptions

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

Ivanidzo4ka · 2019-02-15T01:29:34Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

-            float l2RegularizerWeight = AveragedLinearArguments.AveragedDefaultArgs.L2RegularizerWeight,
-            int numIterations = AveragedLinearArguments.AveragedDefaultArgs.NumIterations)
+            float learningRate = AveragedLinearOptions.AveragedDefaultArgs.LearningRate,
+            bool decreaseLearningRate = AveragedLinearOptions.AveragedDefaultArgs.DecreaseLearningRate,


AveragedDefaultArgs [](start = 62, length = 19)

Would rename to AveragedDefaults same for base class for this class. Just remove Arg. #Resolved

codecov · 2019-02-15T02:39:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3456bb3). Click here to learn what that means.
The diff coverage is 70.09%.

@@            Coverage Diff            @@
##             master    #2563   +/-   ##
=========================================
  Coverage          ?    71.5%           
=========================================
  Files             ?      801           
  Lines             ?   142036           
  Branches          ?    16151           
=========================================
  Hits              ?   101558           
  Misses            ?    36005           
  Partials          ?     4473

Flag	Coverage Δ
#Debug	`71.5% <70.09%> (?)`
#production	`67.78% <65.62%> (?)`
#test	`85.56% <96.1%> (?)`

artidoro · 2019-02-15T08:39:58Z

src/Microsoft.ML.Data/Training/EarlyStoppingCriteria.cs

@@ -62,7 +62,7 @@ public abstract class ArgumentsBase { }
            }
        }

-        internal EarlyStoppingCriterion(TArguments args, bool lowerIsBetter)
+        internal EarlyStoppingCriterion(TOptions args, bool lowerIsBetter)


args [](start = 49, length = 4)

Could you rename the local variable names as well? Here and in the rest of the file? #Resolved

fixed.

fyi.. in this PR the focus is scrubbing the public surface area for Options options .

There are still lots of internal occurrences of Arguments args .. Those are P2 since they can be renamed post V1.0 as well

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

artidoro · 2019-02-15T08:41:44Z

src/Microsoft.ML.Data/Utils/LossFunctions.cs

@@ -176,7 +176,7 @@ public sealed class Arguments : ISupportSdcaClassificationLossFactory, ISupportC
        private const Float Threshold = 0.5f;
        private readonly Float _margin;

-        internal HingeLoss(Arguments args)
+        internal HingeLoss(Options args)


args [](start = 35, length = 4)

Same here, I guess this is a comment that applies to all the files where you rename Arguments to Options. #Resolved

fixed.

fyi.. in this PR the focus is scrubbing the public surface area for Options options .

There are still lots of internal occurrences of Arguments args .. Those are P2 since they can be renamed post V1.0 as well

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

Ivanidzo4ka · 2019-02-15T21:07:07Z

    public class ArgumentsCore

I believe this one would be handled by @artidoro PR #Resolved

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:336 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

abgoswam · 2019-02-15T21:35:43Z

    public class ArgumentsCore

@artidoro could you kindly confirm (want to make sure things do not slip through the cracks)

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

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:336 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

artidoro · 2019-02-15T21:43:19Z

Yes I am merging the two classes Arguments and ArgumentsCore into a single Options class. #Resolved

artidoro · 2019-02-15T21:45:04Z

    public class ArgumentsCore

Sorry the github comment went under discussion. I am taking care of this in my other PR.

In reply to: 464209630 [](ancestors = 464209630,464199502)

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:336 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

Ivanidzo4ka

artidoro · 2019-02-15T22:30:22Z

src/Microsoft.ML.Data/Training/EarlyStoppingCriteria.cs

@@ -89,7 +89,7 @@ protected bool CheckBestScore(Float score)
    public sealed class TolerantEarlyStoppingCriterion : EarlyStoppingCriterion<TolerantEarlyStoppingCriterion.Arguments>
    {
        [TlcModule.Component(FriendlyName = "Tolerant (TR)", Name = "TR", Desc = "Stop if validation score exceeds threshold value.")]
-        public class Arguments : ArgumentsBase, IEarlyStoppingCriterionFactory
+        public class Arguments : OptionsBase, IEarlyStoppingCriterionFactory


Arguments [](start = 21, length = 9)

Could you rename this to Options? #Resolved

artidoro · 2019-02-15T22:30:34Z

src/Microsoft.ML.Data/Training/EarlyStoppingCriteria.cs

@@ -126,7 +126,7 @@ public override bool CheckScore(Float validationScore, Float trainingScore, out

    public abstract class MovingWindowEarlyStoppingCriterion : EarlyStoppingCriterion<MovingWindowEarlyStoppingCriterion.Arguments>
    {
-        public class Arguments : ArgumentsBase
+        public class Arguments : OptionsBase


Arguments [](start = 21, length = 9)

Same here #Resolved

artidoro · 2019-02-15T22:33:24Z

src/Microsoft.ML.Ensemble/OutputCombiners/MultiVoting.cs

@@ -33,7 +33,7 @@ private static VersionInfo GetVersionInfo()
                loaderAssemblyName: typeof(MultiVoting).Assembly.FullName);
        }

-        private sealed class Arguments : ArgumentsBase


Arguments [](start = 29, length = 9)

Even if it's private would be good to rename this one too. #Resolved

i am not touching the private ones in this PR.. (i think i mentioned this in my reply to the other comment)

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

artidoro · 2019-02-15T22:34:33Z

    public sealed class Arguments : ArgumentsBase

Could you rename here too? Just for consistency but it's not public again. #Resolved

Refers to: src/Microsoft.ML.Ensemble/Trainer/Multiclass/MulticlassDataPartitionEnsembleTrainer.cs:39 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

abgoswam · 2019-02-15T22:36:29Z

    public sealed class Arguments : ArgumentsBase

maybe in a separate PR . as discussed first priority is to scrub public surface first.

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

Refers to: src/Microsoft.ML.Ensemble/Trainer/Multiclass/MulticlassDataPartitionEnsembleTrainer.cs:39 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

abgoswam · 2019-02-15T22:37:27Z

    public class ArgumentsCore

ok. sounds good

In reply to: 464212892 [](ancestors = 464212892,464209630,464199502)

Refers to: src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs:336 in e9409ca. [](commit_id = e9409ca, deletion_comment = False)

abgoswam · 2019-02-15T22:37:56Z

ok cool. i will keep it as-is in for now

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

artidoro · 2019-02-15T22:45:43Z

The current changes look good! As mentioned in person, I think we should use the tool to get a list of all the objects in the public surface of ML.NET and see if any of them includes Arguments or Args in their name. Until we do that it is hard to determine whether we have truly gotten rid of them.

If you could run the tool and provide an output file that we can review, it would be extremely useful and we could actually consider this task as being done.

In the meantime let's check in this PR and avoid more conflicts. #Resolved

artidoro · 2019-02-15T22:46:58Z

Please do address the few renaming suggestions I made for public classes. #Resolved

abgoswam · 2019-02-15T23:53:44Z

Sure. i have taken care of the open comments.

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

abgoswam · 2019-02-15T23:54:40Z

Sure. I will run a sanity check with the tool, so we know we have caught all of them. Till then I will keep issue open.

Thanks for the suggestion.

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

abgoswam · 2019-02-16T01:30:15Z

Thanks folks for the review comments!

inital commit. all tests pass locally

553e016

abgoswam requested review from Ivanidzo4ka and artidoro February 15, 2019 01:25

Ivanidzo4ka reviewed Feb 15, 2019

View reviewed changes

abgoswam added 2 commits February 15, 2019 01:50

fix merge conflicts; took care of reviewer comments

35980d6

more merge conflicts

65d733d

artidoro reviewed Feb 15, 2019

View reviewed changes

abgoswam added 2 commits February 15, 2019 19:52

review comments

8b8b8fa

more renamings

e9409ca

Ivanidzo4ka approved these changes Feb 15, 2019

View reviewed changes

artidoro reviewed Feb 15, 2019

View reviewed changes

artidoro approved these changes Feb 15, 2019

View reviewed changes

review comments

7af2eb7

abgoswam added 2 commits February 16, 2019 00:16

Merge branch 'master' into abgoswam/arguments

3896a68

update to latest master

baf0d67

abgoswam merged commit 1f90f50 into dotnet:master Feb 16, 2019

abgoswam deleted the abgoswam/arguments branch February 20, 2019 16:58

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In the public surface area, all occurrences of Argument keyword replaced with Options #2563

In the public surface area, all occurrences of Argument keyword replaced with Options #2563

abgoswam commented Feb 15, 2019

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam Feb 15, 2019

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam Feb 15, 2019

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam Feb 15, 2019

Ivanidzo4ka commented Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam commented Feb 15, 2019

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 15, 2019

Ivanidzo4ka left a comment

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam Feb 15, 2019

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam commented Feb 15, 2019 •

edited

Loading

abgoswam commented Feb 15, 2019

abgoswam commented Feb 15, 2019

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam commented Feb 15, 2019

abgoswam commented Feb 15, 2019

abgoswam commented Feb 16, 2019

In the public surface area, all occurrences of Argument keyword replaced with Options #2563

In the public surface area, all occurrences of Argument keyword replaced with Options #2563

Conversation

abgoswam commented Feb 15, 2019

Ivanidzo4ka Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Ivanidzo4ka Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 15, 2019

Choose a reason for hiding this comment

Ivanidzo4ka Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2019 • edited Loading

Codecov Report

artidoro Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 15, 2019

Choose a reason for hiding this comment

artidoro Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 15, 2019

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 15, 2019 • edited by abgoswam Loading

abgoswam commented Feb 15, 2019

artidoro commented Feb 15, 2019 • edited by abgoswam Loading

artidoro commented Feb 15, 2019

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

artidoro Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

artidoro Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

artidoro Feb 15, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Feb 15, 2019

Choose a reason for hiding this comment

artidoro commented Feb 15, 2019 • edited by abgoswam Loading

abgoswam commented Feb 15, 2019 • edited Loading

abgoswam commented Feb 15, 2019

abgoswam commented Feb 15, 2019

artidoro commented Feb 15, 2019 • edited by abgoswam Loading

artidoro commented Feb 15, 2019 • edited by abgoswam Loading

abgoswam commented Feb 15, 2019

abgoswam commented Feb 15, 2019

abgoswam commented Feb 16, 2019

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 15, 2019 •

edited by abgoswam

Loading

codecov bot commented Feb 15, 2019 •

edited

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 15, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro Feb 15, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

abgoswam commented Feb 15, 2019 •

edited

Loading

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading

artidoro commented Feb 15, 2019 •

edited by abgoswam

Loading