Replaces ChooseColumnsTransform and DropColumnsTransform with SelectColumnsTransform by singlis · Pull Request #1371 · dotnet/machinelearning

singlis · 2018-10-25T01:22:48Z

Replaces ChooseColumnsTransform and DropColumnsTransform with SelectColumnsTransform.

These changes include:

Updates to SelectColumnsTransform to respect ordering when keeping
columns. For example, if the input is ABC and CB is selected, the output
will be CB.
Updates to code that used Choose or Drop columns, replacing with
SelectColumns.
Updates to baseline output for tests to pass
Re-enabled the SavePipeline tests

This fixes #1342
These changes are also related to #754

replacing them with SelectColumnsTransform. These changes include: * Updates to SelectColumnsTransform to respect ordering when keeping columns. For example, if the input is ABC and CB is selected, the output will be CB. * Updates to code that used Choose or Drop columns, replacing with SelectColumns. * Updates to baseline output for tests to pass * Re-enabled the SavePipeline tests This fixes dotnet#1342 These changes are also related to dotnet#754

yaeldekel · 2018-10-25T16:19:23Z

        public bool KeepHidden = true;

The default behavior of ChooseTransform was the opposite of this. We should consider whether we want to change it here as well.
Also, in all the components that use Choose/Drop columns (such as NAHandle, TermLookup, etc.) we should verify that the behavior stays the same, i.e. that no extra columns are dropped/kept.
#Resolved

Refers to: src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs:182 in 99e8aa5. [](commit_id = 99e8aa5, deletion_comment = False)

yaeldekel · 2018-10-25T16:28:19Z

        //env.CheckDecode(cbFloat == sizeof(Float));

Should this be uncommented? #Resolved

Refers to: src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs:235 in 99e8aa5. [](commit_id = 99e8aa5, deletion_comment = False)

yaeldekel · 2018-10-25T16:33:58Z

+                    // finding the associated index that should be used.
+                    foreach(var columnName in selectedColumns)
+                    {
+                        if (columnDict.TryGetValue(columnName, out List<int> columnList))


TryGetValue [](start = 39, length = 11)

I think this should fail if the schema doesn't contain a column by that name. #Resolved

Are you referring to in general or specifically in this case? In general there is an ignoreMissing parameter that when it is true will ignore any columns specified not being in the input. This is checked earlier before the Mapper is created.

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

Oh, I see. I didn't notice the IgnoreMissing argument. In that case, are we sure we want it to default to true?

In reply to: 228257636 [](ancestors = 228257636,228247404)

If it defaults to true, then that means that if the user makes a mistake in the column name, they will not get an error message about it.

In reply to: 228264118 [](ancestors = 228264118,228257636,228247404)

@Zruty0 - Yael makes a great point! What are your thoughts on defaulting ignoreMissing to false? #Resolved

I agree

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

Ok I will default ignoreMissing to false. #Resolved

ignoreMissing now defaults to false.

In reply to: 228264779 [](ancestors = 228264779,228264118,228257636,228247404)

singlis · 2018-10-25T17:06:32Z

        public bool KeepHidden = true;

I feel the same -- Pete preferred this to be KeepHidden as the default. So there are places in code where I have to explicitly specify the drop. Lets bring Pete into the conversation.
@Zruty0 - Can you explain further on why KeepHidden should default to true instead of false?

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

Refers to: src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs:182 in 99e8aa5. [](commit_id = 99e8aa5, deletion_comment = False)

singlis · 2018-10-25T17:06:57Z

        //env.CheckDecode(cbFloat == sizeof(Float));

Yes

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

Refers to: src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs:235 in 99e8aa5. [](commit_id = 99e8aa5, deletion_comment = False)

Ivanidzo4ka · 2018-10-25T17:30:55Z

            return transform.Transform(input);
        }

+        public static IDataTransform CreateKeep(IHostEnvironment env, IDataView input, params string[] keepColumns)


IDataTransform CreateKeep [](start = 22, length = 25)

Do you have any place where it requires to be IDataTransform and not IDataView?
If not, I would suggest you to not use arguments class and just create SelectColumnsTransform and pass required parameters into constructor and then call .Transform on input.
#Resolved

So the signature is this:

public SelectColumnsTransform(IHostEnvironment env, string[] keepColumns, string[] dropColumns, bool keepHidden=true, bool ignoreMissing=true)

where either the keep columns or drop columns can be set. So I used the CreateKeep and CreateDrop so that its really clear as to what the select is doing.
#Resolved

A couple of thoughts I have on this:

Have CreateKeep and CreateDrop return a SelectColumnsTransform. Then you can call Transform on it with the input ( SelectColumnsTransform.CreateKeep(...).Transform(input).

Change the signature for SelectColumnsTransform to instead take an enumerated type of Keep or Drop (i.e. SelectColumnsTransform(env, Select.Keep, cols,...) or SelectColumnsTransform(env, Select.Drop, cols,...)

I like the #2 option as it would fall more naturally with how we tend to work with Transforms and would fall inline with what you are mentioning.

Thoughts? #Resolved

Sorry Ivan, I misunderstood - so the feedback is to not use Arguments inside the function and return an IDataView instead of IDataTransform. Got it, I will make those changes. #Resolved

I was able to remove the usage of Arguments however I was not able to move to an IDataView. There are other transforms that will use this transform and expect to return an IDataTransform.

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

singlis · 2018-10-25T17:58:52Z

FYI - the build failed on the mac debug build but passed on all other platforms which leads me to believe this is one of the intermittent test failures that we see on the mac. I am following up with @TomFinley to confirm since he is investigating this issue. #Resolved

shauheen · 2018-10-26T18:50:43Z

@singlis can you update the branch so that we get a fresh build? #Resolved

singlis · 2018-10-26T18:51:51Z

yes - will do. #Resolved

Zruty0 · 2018-10-29T17:49:47Z

-            var view = new ChooseColumnsTransform(host, args, lookup);
+            args.Column = copyColumns.ToArray();
+
+            var view = CopyColumnsTransform.Create(host, args, lookup);


CopyColumnsTransform [](start = 23, length = 20)

can we use a constructor instead of args-based Create ? #Resolved

Zruty0

Zruty0 · 2018-10-29T17:52:25Z

        public bool KeepHidden = true;

I don't remember a specific reason. Probably, let's do the least destructive conversion, so preserve whatever ChooseColumns was doing by default

In reply to: 433130838 [](ancestors = 433130838,433115221)

Refers to: src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs:182 in 99e8aa5. [](commit_id = 99e8aa5, deletion_comment = False)

default behavior of ChooseColumns. - Addressed other feedback such as removing use of Argument classes.

Ivanidzo4ka · 2018-10-29T23:00:02Z


            [Argument(ArgumentType.AtMostOnce, HelpText = "Specifies whether to keep or remove hidden columns.", ShortName = "hidden", SortOrder = 3)]
-            public bool KeepHidden = true;
+            public bool KeepHidden = false;


false [](start = 37, length = 5)

nit: We usually have Defaults class with const properties, which ease a bit spreading of parameters across code. #Resolved

Ivanidzo4ka · 2018-10-29T23:08:20Z

+using Xunit;
+using Xunit.Abstractions;
+
+namespace Microsoft.ML.Tests


Microsoft.ML.Tests [](start = 10, length = 18)

nit: it's Microsoft.ML.Tests.Transformers in other tests. #Resolved

Ivanidzo4ka

- Select with drop respects keepHidden, defaults to false. - When loading in a KeepColumns transform, defaults keepHidden to true for back compat. - Added additional tests

…g for other reasons

singlis added 2 commits October 24, 2018 18:14

Merge remote-tracking branch 'origin/master' into singlis/drop4

99e8aa5

singlis requested review from Ivanidzo4ka, Zruty0 and yaeldekel October 25, 2018 01:22

yaeldekel reviewed Oct 25, 2018

View reviewed changes

Comment thread src/Microsoft.ML.Data/Commands/SaveDataCommand.cs Outdated

yaeldekel reviewed Oct 25, 2018

View reviewed changes

Ivanidzo4ka reviewed Oct 25, 2018

View reviewed changes

Comment thread src/Microsoft.ML.Data/Evaluators/AnomalyDetectionEvaluator.cs Outdated

Ivanidzo4ka reviewed Oct 25, 2018

View reviewed changes

Comment thread src/Microsoft.ML.Data/Transforms/SelectColumnsTransform.cs

Ivanidzo4ka reviewed Oct 25, 2018

View reviewed changes

singlis added 2 commits October 26, 2018 18:26

Merge remote-tracking branch 'origin/master' into singlis/drop4

6aabcf4

* Updating based on feedback

8aee4e2

Zruty0 reviewed Oct 29, 2018

View reviewed changes

Zruty0 approved these changes Oct 29, 2018

View reviewed changes

- ignoreMissing and keepHidden now default to false to follow the

44b5b3b

default behavior of ChooseColumns. - Addressed other feedback such as removing use of Argument classes.

Ivanidzo4ka reviewed Oct 29, 2018

View reviewed changes

Ivanidzo4ka approved these changes Oct 29, 2018

View reviewed changes

singlis added 4 commits October 29, 2018 17:07

- Replacing defaults with a Defaults class

dd0fe4a

- Select with drop respects keepHidden, defaults to false. - When loading in a KeepColumns transform, defaults keepHidden to true for back compat. - Added additional tests

Merge remote-tracking branch 'origin/master' into singlis/drop4

17d7298

Merge remote-tracking branch 'origin/master' into singlis/drop4

5371f02

Merge remote-tracking branch 'origin/master' into singlis/drop4

0e1f939

singlis added 2 commits October 30, 2018 13:18

- Disabling SavePipeHash and SavePipeLabel tests as these are failin…

373bd18

…g for other reasons

Merge remote-tracking branch 'origin/master' into singlis/drop4

6e798b1

singlis merged commit c2b5e76 into dotnet:master Oct 30, 2018

singlis deleted the singlis/drop4 branch March 7, 2019 00:54

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

Uh oh!

Conversation

singlis commented Oct 25, 2018

Uh oh!

Uh oh!

yaeldekel commented Oct 25, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaeldekel commented Oct 25, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yaeldekel Oct 25, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

yaeldekel Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

yaeldekel Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

singlis Oct 25, 2018 • 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!

Zruty0 Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

singlis Oct 29, 2018 • 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!

singlis Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

singlis commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singlis commented Oct 25, 2018

Uh oh!

Uh oh!

Uh oh!

Ivanidzo4ka Oct 25, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singlis Oct 27, 2018 • 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!

singlis Oct 29, 2018 • 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!

singlis Oct 29, 2018 • 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!

singlis Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

singlis commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shauheen commented Oct 26, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singlis commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zruty0 Oct 29, 2018 • edited by singlis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 left a comment

Choose a reason for hiding this comment

Uh oh!

yaeldekel commented Oct 25, 2018 •

edited by singlis

Loading

yaeldekel commented Oct 25, 2018 •

edited by singlis

Loading

yaeldekel Oct 25, 2018 •

edited by singlis

Loading

singlis Oct 25, 2018 •

edited

Loading

singlis Oct 29, 2018 •

edited

Loading

singlis commented Oct 25, 2018 •

edited

Loading

Ivanidzo4ka Oct 25, 2018 •

edited by singlis

Loading

singlis Oct 27, 2018 •

edited

Loading

singlis Oct 29, 2018 •

edited

Loading

singlis Oct 29, 2018 •

edited

Loading

singlis commented Oct 25, 2018 •

edited

Loading

shauheen commented Oct 26, 2018 •

edited by singlis

Loading

singlis commented Oct 26, 2018 •

edited

Loading

Zruty0 Oct 29, 2018 •

edited by singlis

Loading

Ivanidzo4ka Oct 29, 2018 •

edited by singlis

Loading

Ivanidzo4ka Oct 29, 2018 •

edited by singlis

Loading