Added convenience constructors for ScoreTransform and TrainAndScoreTransform. #614

zeahmed · 2018-07-31T00:21:57Z

This PR fixes #606.

…ansform.

zeahmed · 2018-07-31T00:22:44Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs

+            return Create(env, args, args.Trainer.CreateInstance(env), input);
+        }
+
+        private static IDataTransform Create(IHostEnvironment env, Arguments args, ITrainer trainer, IDataView input)


Moving the common code in private create method. #Closed

Actually, a more common pattern is to use AssertValue in private methods, and CheckValue in public methods.

The purpose of the Check* is twofold: first, we want to enforce the calling contract, and second, we want to actually EXPLICITLY SPECIFY the calling contract.
So that you can look at any argument and immediately, within 3 lines of code, tell whether it can be null or not etc.

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

Moved all the checks in the public methods instead.

In reply to: 206367797 [](ancestors = 206367797,206362456)

Ivanidzo4ka · 2018-07-31T00:37:01Z

            "Trainer cannot be null. If your model is already trained, please use ScoreTransform instead.");

it should move to upper call. no reason to check trainer here if you already pass it. #Closed

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:190 in 76bab69. [](commit_id = 76bab69, deletion_comment = False)

Ivanidzo4ka · 2018-07-31T00:45:08Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs

+        /// <param name="featureColumn">Role name for features.</param>
+        /// <param name="labelColumn">Role name for label.</param>
+        /// <returns></returns>
+        public static IDataTransform Create(IHostEnvironment env, IDataView input, ITrainer trainer, string featureColumn = DefaultColumnNames.Features, string labelColumn = DefaultColumnNames.Label)


labelColumn [](start = 160, length = 11)

don't you need custom columns here?
also if you can split this long line into two, it would be nice. #Resolved

It can be but I see CustomColumns as advanced argument (not frequently used). Let see if other reviewers have same argument.

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

well, it's not only CustomColumns, but also Weight, Group, Name, considering what they all have default values and user don't have to specify them, I don't see why not to add them, but I would also hear other people opinion. Just in case, same applies for Group column and CustomColumns in Scorer

In reply to: 206366117 [](ancestors = 206366117,206365422)

Let see if @TomFinley has any suggestion in this regard? #Resolved

Thank you for the question @zeahmed. My tendency would be to view group as something possibly useful. Weight and name, I view as less fundamentally useful. Custom columns, I am somewhat reluctant to support because I have some difficulty imagining how to explain that one to people. :P

However, I cannot quite claim to care that much. not because it isn't an important consideration but because the proposal of #581 renders it moot. We're going to be moving towards a world where, essentially, all trainers become TrainAndScore transforms, with more local decisions about what column roles are and aren't important.

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

Zruty0 · 2018-07-31T01:00:43Z

            "Trainer cannot be null. If your model is already trained, please use ScoreTransform instead.");

This is a public method though, so it can be called directly. Let's keep the check.

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

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:190 in 76bab69. [](commit_id = 76bab69, deletion_comment = False)

Zruty0 · 2018-07-31T01:01:23Z

            "Trainer cannot be null. If your model is already trained, please use ScoreTransform instead.");

Actually, Ivan's point is that checking the trainer can happen inside the other create, not validation of the input

In reply to: 409059591 [](ancestors = 409059591,409056026)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:190 in 76bab69. [](commit_id = 76bab69, deletion_comment = False)

Zruty0

🕐

Ivanidzo4ka · 2018-07-31T01:04:45Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

can we use nameof() here and for groupColumn? #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

Ivanidzo4ka · 2018-07-31T01:06:02Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs

+
+            return Create(env, args, trainer, input);
+        }
+
        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


input [](start = 92, length = 5)

can you check input in this function? #Resolved

Ivanidzo4ka · 2018-07-31T01:06:43Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs

+                LabelColumn = labelColumn
+            };
+
+            return Create(env, args, trainer, input);


trainer, input [](start = 37, length = 14)

Can you check trainer and input? #Resolved

zeahmed · 2018-07-31T01:29:03Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

Dont get you. You mean check here for FeatureColumn and GroupColumn? Both have default values in Argument class so no need to check...Let me know if you mean something else?

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

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

zeahmed · 2018-07-31T01:33:31Z

            "Trainer cannot be null. If your model is already trained, please use ScoreTransform instead.");

Moved all the checks to public method instead. Let me know if private one do need checking as well?

In reply to: 409059697 [](ancestors = 409059697,409059591,409056026)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:190 in 76bab69. [](commit_id = 76bab69, deletion_comment = False)

Ivanidzo4ka · 2018-07-31T01:35:42Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

var weight = TrainUtils.MatchNameOrDefaultOrNull(ectx, schema, nameof(args.WeightColumn), args.WeightColumn,
DefaultColumnNames.Weight);
see 3rd parameter, it's not a string literal, but actual name of parameter.

In reply to: 409063977 [](ancestors = 409063977,409060224)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

zeahmed · 2018-07-31T01:40:55Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

So why the "featureColumn" starts with lower case "f" here? if I use nameof(args.FeatureColumn) then it will be "FeatureColumn". Will it be correct?

In reply to: 409065068 [](ancestors = 409065068,409063977,409060224)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

Ivanidzo4ka · 2018-07-31T01:53:49Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

Because someone make mistake, or initially we had internal variable featureColumn, but we switch do direct call to argument field. Who knows?
Actually, went to internal repo and use blame.
Tom Capitalize our arguments at some point, but string literals remain same.
By using nameof() you remove this problem of keeping them in sync ;)

In reply to: 409065784 [](ancestors = 409065784,409065068,409063977,409060224)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

zeahmed · 2018-07-31T17:05:59Z

                "featureColumn", args.FeatureColumn, DefaultColumnNames.Features);

hmm...just changed then...:)

In reply to: 409067891 [](ancestors = 409067891,409065784,409065068,409063977,409060224)

Refers to: src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs:86 in a3a0302. [](commit_id = a3a0302, deletion_comment = False)

Ivanidzo4ka

Zruty0 · 2018-07-31T18:17:06Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs

+        {
+            Contracts.CheckValue(env, nameof(env));
+            env.CheckValue(input, nameof(input));
+            env.CheckValue(trainer, nameof(trainer));


env.CheckValue(trainer, nameof(trainer)); [](start = 12, length = 41)

in the interest of self-documenting functions, add CheckValue (or CheckValueOrNull) on featureColumn and labelColumn #Resolved

Zruty0 · 2018-07-31T18:18:07Z

src/Microsoft.ML.Data/Transforms/TrainAndScoreTransform.cs


+        private static IDataTransform Create(IHostEnvironment env, Arguments args, ITrainer trainer, IDataView input)
+        {


Here, AssertValue of all the things you CheckValue'd in the public side.

This is again in the interest of self-documenting methods #Resolved

Zruty0

zeahmed · 2018-07-31T22:36:43Z

Just got to know that these transform will go away in new API...:) #Closed

TomFinley

Added convenience constructors for ScoreTransform and TrainAndScoreTr…

76bab69

…ansform.

zeahmed requested review from Ivanidzo4ka, TomFinley and Zruty0 July 31, 2018 00:21

zeahmed commented Jul 31, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 31, 2018

View reviewed changes

zeahmed added 2 commits July 30, 2018 17:52

Moving validation/checking of arguments to relevant blocks.

a3a0302

Removed <Returns> tag from doc section.

1828860

Zruty0 suggested changes Jul 31, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 31, 2018

View reviewed changes

Addressed reviewers' comments.

ac4e76a

Ivanidzo4ka approved these changes Jul 31, 2018

View reviewed changes

Zruty0 reviewed Jul 31, 2018

View reviewed changes

Zruty0 approved these changes Jul 31, 2018

View reviewed changes

Addressed reviewers' comments.

6b1616c

TomFinley approved these changes Jul 31, 2018

View reviewed changes

Addressed reviewers' comments.

aca4a2e

shauheen merged commit 4bd866e into dotnet:master Aug 1, 2018

zeahmed deleted the CC_Train_and_score branch January 30, 2019 21:30

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added convenience constructors for ScoreTransform and TrainAndScoreTransform. #614

Added convenience constructors for ScoreTransform and TrainAndScoreTransform. #614

zeahmed commented Jul 31, 2018

zeahmed Jul 31, 2018 •

edited by Zruty0

Loading

Zruty0 Jul 31, 2018

zeahmed Jul 31, 2018

Ivanidzo4ka commented Jul 31, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

zeahmed Jul 31, 2018

Ivanidzo4ka Jul 31, 2018

zeahmed Jul 31, 2018 •

edited

Loading

TomFinley Jul 31, 2018

Zruty0 commented Jul 31, 2018

Zruty0 commented Jul 31, 2018

Zruty0 left a comment

Ivanidzo4ka commented Jul 31, 2018 •

edited by zeahmed

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

zeahmed commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka left a comment

Zruty0 Jul 31, 2018 •

edited by zeahmed

Loading

Zruty0 Jul 31, 2018 •

edited by zeahmed

Loading

Zruty0 left a comment

zeahmed commented Jul 31, 2018 •

edited

Loading

TomFinley left a comment


		private static IDataTransform Create(IHostEnvironment env, Arguments args, ITrainer trainer, IDataView input)
		{

Added convenience constructors for ScoreTransform and TrainAndScoreTransform. #614

Added convenience constructors for ScoreTransform and TrainAndScoreTransform. #614

Conversation

zeahmed commented Jul 31, 2018

zeahmed Jul 31, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Zruty0 Jul 31, 2018

Choose a reason for hiding this comment

zeahmed Jul 31, 2018

Choose a reason for hiding this comment

Ivanidzo4ka commented Jul 31, 2018 • edited by Zruty0 Loading

Ivanidzo4ka Jul 31, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

zeahmed Jul 31, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Jul 31, 2018

Choose a reason for hiding this comment

zeahmed Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Jul 31, 2018

Choose a reason for hiding this comment

Zruty0 commented Jul 31, 2018

Zruty0 commented Jul 31, 2018

Zruty0 left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Jul 31, 2018 • edited by zeahmed Loading

Ivanidzo4ka Jul 31, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

Ivanidzo4ka Jul 31, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

zeahmed commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka commented Jul 31, 2018

zeahmed commented Jul 31, 2018

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Zruty0 Jul 31, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

Zruty0 Jul 31, 2018 • edited by zeahmed Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

zeahmed commented Jul 31, 2018 • edited Loading

TomFinley left a comment

Choose a reason for hiding this comment

zeahmed Jul 31, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka commented Jul 31, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

zeahmed Jul 31, 2018 •

edited

Loading

Ivanidzo4ka commented Jul 31, 2018 •

edited by zeahmed

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

Ivanidzo4ka Jul 31, 2018 •

edited by zeahmed

Loading

Zruty0 Jul 31, 2018 •

edited by zeahmed

Loading

Zruty0 Jul 31, 2018 •

edited by zeahmed

Loading

zeahmed commented Jul 31, 2018 •

edited

Loading