Make array argument names plural #2124

najeeb-kazmi · 2019-01-11T18:14:31Z

Fixes #2040

Makes array argument names plural where they were singular.

Fixes CmdParser to check for Attribute name in addition to field Name.

Fixes Entry Points InputBuilder to check for both Attribute and Field names.

src/Microsoft.ML.Data/Commands/CrossValidationCommand.cs

abgoswam · 2019-01-11T19:18:23Z

For the 3 places you mention, where we used string Terms the convention proposed in the PR seems confusing to me.

In particular public string TermsList looks odd. Isn't it ?

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

sfilipi · 2019-01-11T23:14:33Z

src/Microsoft.ML.EntryPoints/CrossValidationMacro.cs

@@ -95,7 +95,7 @@ public sealed class Output
        {
            [TlcModule.Output(Desc = "The final model including the trained predictor model and the model from the transforms, " +
                "provided as the Input.TransformModel.", SortOrder = 1)]
-            public PredictorModel[] PredictorModel;
+            public PredictorModel[] PredictorModels;


PredictorModels [](start = 36, length = 15)

i think this might have side-effects in the entry points; but that is probably ok.

@yaeldekel might have an opinion too.

You're right, it did break some entry point tests. I realize this one is not an ArgumentAttribute to begin with, so I'm reverting it in my next change (I haven't pushed yet).

Many other entry point tests also broke because of renaming Column to Columns and setting Name = "Column". Basically, the graph runner ctor checks for validity of node names, but it only checks against ArgumentAttribute.Name and the ArgumentAttribute.Aliases, and since Name was set to "Column" and Aliases were the ShortName list, all of those checks failed.

It wouldn't have been correct to change ShortName to "col,Columnsas that is visible on the command line, and "Columns" doesn't make sense on the command line. I ended up modifying the check inInputBuilder`.

sfilipi · 2019-01-11T23:16:04Z

src/Microsoft.ML.EntryPoints/MacroUtils.cs

@@ -75,21 +75,21 @@ public static EvaluateInputBase GetEvaluatorArgs(TrainerKinds kind, out string e
        public sealed class ArrayIPredictorModelInput
        {
            [Argument(ArgumentType.Required, HelpText = "The models", SortOrder = 1)]
-            public PredictorModel[] Model;
+            public PredictorModel[] Models;


Models [](start = 36, length = 6)

Same note on tis file, about entry points.

Also, the EntryPoint project is not directly user facing.. idk if we still want to do it for consistency.

This one does't break the tests, but I do agree we might want to rethink touching entry point arguments right now. The effects might propagate to NimbusML for instance.

@TomFinley what do you think?

…and ep tests

najeeb-kazmi · 2019-01-16T23:18:06Z

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

-            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
-            public string Terms;
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", Name = "Terms", Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
+            public string Term;


public [](start = 12, length = 6)

To be made internal in #2133, after #2059 gets merged

najeeb-kazmi · 2019-01-16T23:19:47Z

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

-            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", SortOrder = 105, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
-            public string Terms;
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", Name = "Terms", SortOrder = 105, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
+            public string Term;


public [](start = 12, length = 6)

To be made internal in #2133, after #2059 gets merged

najeeb-kazmi · 2019-01-16T23:20:27Z

src/Microsoft.ML.Transforms/Text/WordBagTransform.cs

-        [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", SortOrder = 1, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
-        public string Terms;
+        [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of terms", Name = "Terms", SortOrder = 1, Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
+        public string Term;


public [](start = 8, length = 6)

To be made internal in #2133, after #2059 gets merged

najeeb-kazmi · 2019-01-16T23:20:54Z

src/Microsoft.ML.Transforms/Text/StopWordsRemovingTransformer.cs

-            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of stopwords", Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
-            public string Stopwords;
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Comma separated list of stopwords", Name = "Stopwords", Visibility = ArgumentAttribute.VisibilityType.CmdLineOnly)]
+            public string Stopword;


public [](start = 12, length = 6)

To be made internal in #2133, after #2059 gets merged

Hi Najeeb -- Name is an override and not an alias right?

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

Name is an alias (with my changes to the cmd line runner). I'm adding the Name attribute so that behavior of existing command lines doesn't get affected with renaming of the field name.

singlis · 2019-01-18T01:58:44Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

@@ -477,6 +477,11 @@ private static ArgumentInfo GetArgumentInfo(Type type, object defaults)
                        throw Contracts.Except("Duplicate default argument '{0}' vs '{1}'", def.LongName, field.Name);

                    string name = ArgCase(field.Name);
+
+                    string attrName = null;
+                    if (attr.Name != null && attr.Name.ToLowerInvariant() != name.ToLowerInvariant())


Do you care about empty strings too? If so, I would use String.IsEmptyOrNull.
Also you can do a case insensitive compare with String.Equals(a,b,StringComparison.OrginalIgnoreCase) #Closed

(!string.IsNullorEmpty(attr.Name) &&
attr.Name.Equals(name, StringComparison.InvariantCultureIgnoreCase)

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

See the impleemntation of ArgumentAttribute.Name; it already guards against this. If you tried to set something to be empty, or even all whitespace, the property value would be null.

singlis · 2019-01-18T01:59:03Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

@@ -495,6 +500,11 @@ private static ArgumentInfo GetArgumentInfo(Type type, object defaults)

                    if (map.ContainsKey(name.ToLowerInvariant()))
                        throw Contracts.Except("Duplicate name '{0}' in argument type '{1}'", name, type.Name);
+                    if (attrName != null)


attrName [](start = 24, length = 8)

Same comment here about empty string

Not applicable here, since this is attrName, not attr.Name. It's going to be either null or non-empty.

singlis · 2019-01-18T02:01:06Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

@@ -495,6 +500,11 @@ private static ArgumentInfo GetArgumentInfo(Type type, object defaults)

                    if (map.ContainsKey(name.ToLowerInvariant()))
                        throw Contracts.Except("Duplicate name '{0}' in argument type '{1}'", name, type.Name);
+                    if (attrName != null)
+                    {
+                        if (map.ContainsKey(attrName.ToLowerInvariant()))


You may also construct the map with a case insensitive string compare, that way you dont have to do a lower variant.

See here: https://stackoverflow.com/questions/13988643/case-insensitive-dictionary-with-string-key-type-in-c-sharp

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

singlis · 2019-01-18T02:04:09Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

@@ -508,6 +518,8 @@ private static ArgumentInfo GetArgumentInfo(Type type, object defaults)

                    // Note that we put the default arg in the map to ensure that no other args use the same name.
                    map.Add(name.ToLowerInvariant(), arg);
+                    if (attrName != null)
+                        map.Add(attrName.ToLowerInvariant(), arg);


ToLowerInvariant [](start = 41, length = 16)

If you can do a case insensitive comparer for the map the toLowerInvariant is not needed. However if you want to convert to lower invariant for other reasons, I would suggest doing the conversion at the top of the function and using that through the rest of the code. That way you do the conversion once.

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

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedLinear.cs

glebuk · 2019-01-18T17:53:25Z

                // Drop columns indexed by args.Index

indices? #Closed

Refers to: src/Microsoft.ML.Data/Dirty/ChooseColumnsByIndexTransform.cs:86 in df10cb7. [](commit_id = df10cb7, deletion_comment = False)

glebuk · 2019-01-18T17:54:38Z

    [TlcModule.EntryPoint(Name = "Transforms.ScoreColumnSelector", Desc = "Selects only the last score columns and the extra columns specified in the arguments.", UserName = "Choose Columns By Index")]

indices? #Closed

Refers to: src/Microsoft.ML.Data/EntryPoints/ScoreColumnSelector.cs:22 in df10cb7. [](commit_id = df10cb7, deletion_comment = False)

glebuk

🕐

TomFinley · 2019-01-18T22:08:58Z

src/Microsoft.ML.Core/CommandLine/CmdParser.cs

+
+                    string attrName = null;
+                    if (attr.Name != null && attr.Name.ToLowerInvariant() != name.ToLowerInvariant())
+                        attrName = attr.Name;


My expectation is that the logic should be the same as for entry-points... if Name is set, then we would not use the field name from the user's perspective at all.

TomFinley · 2019-01-24T19:51:27Z

Hi @najeeb-kazmi or others, is this being worked on?

najeeb-kazmi · 2019-01-25T00:37:07Z

Hi @najeeb-kazmi or others, is this being worked on?

@TomFinley Yes, I have modified to CmdParser to override field name with with attribute name if it is present. The comamnd line tests that were failing are passing now. Currently, I'm looking into the EntryPoints tests that fail when I override the field name with the attribute name.

…with attribute name if attribute name is present

codecov · 2019-01-25T20:50:19Z

Codecov Report

Merging #2124 into master will increase coverage by <.01%.
The diff coverage is 70.46%.

@@            Coverage Diff             @@
##           master    #2124      +/-   ##
==========================================
+ Coverage   71.17%   71.17%   +<.01%     
==========================================
  Files         780      780              
  Lines      140404   140404              
  Branches    16042    16053      +11     
==========================================
+ Hits        99931    99935       +4     
+ Misses      36023    36021       -2     
+ Partials     4450     4448       -2

Flag	Coverage Δ
#Debug	`71.17% <70.46%> (ø)`	⬆️
#production	`67.56% <66.11%> (ø)`	⬆️
#test	`85.18% <92.85%> (ø)`	⬆️

TomFinley

Thank you for doing this @najeeb-kazmi . Now in both cases both command line and entry-points have the same desired behavior that the name serves as an override to the field name. I see @glebuk is still blocking, perhaps he can take another look.

glebuk

artidoro · 2019-01-30T01:43:25Z

            string[] term = null,

Does it make sense to update it here as well?

Refers to: src/Microsoft.ML.Data/Transforms/ValueToKeyMappingTransformer.cs:187 in ac7ee5a. [](commit_id = ac7ee5a, deletion_comment = False)

artidoro · 2019-01-30T18:45:12Z

Hey @najeeb-kazmi are you still looking into this PR? Would be great to have this in :)

…into 2040

najeeb-kazmi · 2019-02-01T03:25:55Z

Hey @najeeb-kazmi are you still looking into this PR? Would be great to have this in :)

@artidoro yes - currently it's building on my laptop but failing in the CI. Can't figure out why.

artidoro · 2019-02-01T03:28:47Z

Would suggest to remove bin folder, sync with master, and rebuild from command line.

…to 2040

najeeb-kazmi · 2019-02-01T05:13:23Z

Thanks @artidoro for your help in getting the conflicts merged!

Make array argument names plural

a012582

najeeb-kazmi requested review from TomFinley, Ivanidzo4ka, sfilipi, wschin, abgoswam, glebuk and codemzs January 11, 2019 18:14

najeeb-kazmi changed the title ~~Make array argument names plural~~ [WIP] Make array argument names plural Jan 11, 2019

abgoswam reviewed Jan 11, 2019

View reviewed changes

src/Microsoft.ML.Data/Commands/CrossValidationCommand.cs Show resolved Hide resolved

abgoswam reviewed Jan 11, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ValueToKeyMappingTransformer.cs Outdated Show resolved Hide resolved

sfilipi reviewed Jan 11, 2019

View reviewed changes

Fixing CmdParser and EntryPoints InputBuilder to not breaks cmd line …

361e990

…and ep tests

najeeb-kazmi changed the title ~~[WIP] Make array argument names plural~~ Make array argument names plural Jan 16, 2019

najeeb-kazmi commented Jan 16, 2019

View reviewed changes

Merge branch 'master' into 2040

7a25d33

singlis reviewed Jan 18, 2019

View reviewed changes

src/Microsoft.ML.Core/CommandLine/CmdParser.cs Outdated Show resolved Hide resolved

singlis reviewed Jan 18, 2019

View reviewed changes

najeeb-kazmi and others added 2 commits January 17, 2019 18:08

Fix build and tests

11c1642

Merge branch 'master' into 2040

df10cb7

singlis reviewed Jan 18, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ValueToKeyMappingTransformerImpl.cs Show resolved Hide resolved

singlis reviewed Jan 18, 2019

View reviewed changes

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedLinear.cs Outdated Show resolved Hide resolved

glebuk suggested changes Jan 18, 2019

View reviewed changes

TomFinley reviewed Jan 18, 2019

View reviewed changes

najeeb-kazmi and others added 3 commits January 25, 2019 11:56

Fixing CmdParser and EntryPoints InputBuilder to override field name …

7aceae5

…with attribute name if attribute name is present

Merge branch 'master' into 2040

0ea8e60

Fix build errors

ac7ee5a

TomFinley approved these changes Jan 25, 2019

View reviewed changes

glebuk approved these changes Jan 28, 2019

View reviewed changes

najeeb-kazmi added 5 commits January 31, 2019 15:42

Merge branch 'najeeb-kazmi-2040'

521b41d

Fixing merge conflicts

57f80ca

Merge branch 'najeeb-kazmi-2040'

332d318

Merge branch 'najeeb-kazmi-2040' into 2040

04867cb

Merge branch '2040' of https://github.com/najeeb-kazmi/machinelearning …

9bb00fd

…into 2040

najeeb-kazmi added 5 commits January 31, 2019 19:30

Fixing a test

81d660c

Merge branch 'master' of https://github.com/najeeb-kazmi/machinelearning

f1a04b8

Merge branch 'master' into 2040

33bd91f

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

07c3c91

…to 2040

Fix build errors

2523832

najeeb-kazmi merged commit 28ae548 into dotnet:master Feb 1, 2019

najeeb-kazmi deleted the 2040 branch January 30, 2020 01:17

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

Make array argument names plural #2124

Make array argument names plural #2124

Uh oh!

Conversation

najeeb-kazmi commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

abgoswam commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

najeeb-kazmi Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Jan 18, 2019 • edited by glebuk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glebuk commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glebuk commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glebuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jan 24, 2019

Uh oh!

najeeb-kazmi commented Jan 25, 2019

Uh oh!

codecov bot commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

glebuk left a comment

Choose a reason for hiding this comment

Uh oh!

najeeb-kazmi commented Jan 11, 2019 •

edited

Loading

abgoswam commented Jan 11, 2019 •

edited

Loading

najeeb-kazmi Jan 16, 2019 •

edited

Loading

singlis Jan 18, 2019 •

edited by glebuk

Loading

glebuk commented Jan 18, 2019 •

edited

Loading

glebuk commented Jan 18, 2019 •

edited

Loading

codecov bot commented Jan 25, 2019 •

edited

Loading