SelectColumnsTransform drops columns that it should not drop

[yaeldekel]

The KeepHidden argument should keep/drop only hidden columns with the same names as ones that are specified in the arguments. Currently, in the "drop" mode of the transform, it drops all hidden columns, no matter what their name is.

I am not sure this argument should even be relevant for drop mode, I think it should automatically drop all columns with the specified names, whether they are hidden or not.

cc @Zruty0 , @singlis, what do you think?

[singlis]

Thank you Yael for bringing this up!

I don't wholly agree -- mainly because the keepHidden argument is meant to be independent of other arguments. It should not have a different behavior in the drop mode vs in the keep mode. I think if a user was learning about the api for the first time, they would expect this to work as advertised. Having the behavior change because of the mode can be misleading. Of course someone using DropColumns would expect the columns to remain hidden, so this new usage is misleading -- but its past context of using DropColumns that creates that expectation.

As for the argument not being there I think is a good question, I voted for it being there :). If we want to remove it, then we should restructure the constructors as such (that may allow for differentiating the constructors for keep vs drop).

Note too that when we load a model with DropColumns, keepHidden is true.

[yaeldekel]

Thanks Scott!

To make sure I understand what you mean - what don't you agree with? There are two issues:

1. SelectColumnsTransform in drop mode with keepHidden=false shouldn't drop columns with names that are not specified.
2. SelectColumnsTransform in drop mode should ignore the keepHidden argument altogether, since in the keepHidden=true case it might cause "unhiding" of a column that had the same name as the column being dropped

[singlis]

Actually let me think more on #1 -- since you are applying the keepHidden based upon columns not listed, that may make sense.

[singlis]

OK so talking with Yael, the question is whether KeepHidden makes sense when dropping columns. I think this is where we could use Pete's wisdom
@Zruty0 - what are your thoughts on this?

If we don't need keepHidden when dropping in SelectColumns, then I feel we should only expose the argument in the for the keep columns scenario and change the API accordingly. So instead of this:

SelectColumnsTransform(IHostEnvironment env, string[] keepColumns, string[] dropColumns, bool keepHidden)

We would have this:

SelectColumnsTransform(IHostEnvironment env, bool keepHidden, params string[] keepColumns)
SelectColumns(IHostEnvironment env, params string[] dropColumns)

There is also a question on what to do then about the Arguments class as the keepHidden argument is exposed there. In this case, we could have a KeepArguments a DropArguments where KeepArguments would have the keepHidden flag.

[Zruty0]

I like the idea of the 2 separate ctors with different set of args.

For Arguments, I'm less strict: I don't mind either one Arguments class (where KeepHidden will be ignored in case of drop mode), or 2 argument classes.