SelectColumnsTransform drops columns that it should not drop #1504

Activity

1. singlis commented on Nov 1, 2018

   

   singlis

   on Nov 1, 2018

   Member

   More actions

   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.
2. yaeldekel commented on Nov 2, 2018

   

   yaeldekel

   on Nov 2, 2018

   Author

   More actions

   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
3. singlis commented on Nov 2, 2018

   

   singlis

   on Nov 2, 2018

   Last edited by singlis

   Member

   More actions

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

   

   singlis

   on Nov 2, 2018

   Last edited by singlis

   Member

   More actions

   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.
5. Zruty0 commented on Nov 2, 2018

   

   Zruty0

   on Nov 2, 2018

   Contributor

   More actions

   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.
6. yaeldekel

   added a commit that references this issue on Nov 5, 2018

   Update SelectColumnsTransform based on discussion in issue dotnet#1504

   44086d4
7. yaeldekel

   closed this as completedin #1505on Nov 9, 2018
8. yaeldekel

   added a commit that references this issue on Nov 9, 2018

   Don't drop columns that are not specified (#1505)

   ...

   e803520
9. ghost locked as resolved and limited conversation to collaborators on Mar 27, 2022