ColumnCopier API is confusing

[svick]

ColumnCopier has a constructor that takes a sequence of strings and another constructor that takes a sequence of pairs of strings.

This means that both of the following lines are valid, with different meanings compile:

new ColumnCopier("foo", "bar")
new ColumnCopier(("foo", "bar"))

It seems this API is confusing to people who are learning about ML.NET, as evidenced by these two issues on the docs repo: dotnet/docs#5628, dotnet/docs#5671. There is also #189 about the same issue.

Could the API be changed somehow to make it clearer?

One option would be to remove both constructors and make AddColumn fluent. E.g. the two examples above would change to:

new ColumnCopier().AddColumn("foo").AddColumn("bar")
new ColumnCopier().AddColumn("foo", "bar")

This makes the API more verbose, but maybe it would be worth it to make it clearer?

I'm also assuming that API breaking changes are acceptable at this point. If they're not, then there's probably nothing that can be done.

[Ivanidzo4ka]

What version of nuget are you trying? v0.1? @zeahmed reported this issue was closed in v0.2, I'm not sure we release v0.2 version into nuget.org, but you can check daily version on myget:

https://dotnet.myget.org/F/dotnet-core/api/v3/index.json

[svick]

@Ivanidzo4ka I'm looking at the source code in master:

machinelearning/src/Microsoft.ML/CSharpApi.cs

Line 8684 in 9d19d0e

public ColumnCopier(params string[] inputColumns)

machinelearning/src/Microsoft.ML/CSharpApi.cs

Line 8695 in 9d19d0e

public ColumnCopier(params ValueTuple<string, string>[] inputOutputColumns)

As I understand it, #189 was closed, because the code now throws an exception if used incorrectly. But I'm not sure that's good enough, the API is still confusing.

[zeahmed]

@svick, Yes, I strongly agree that the API should be simplified for this case. It's just not ColumnCopier but also a few other components as well which operate this way.

[zeahmed]

Luckily, this API code is auto generated from the following file. So, it should not be difficult to change. Let's think about backward compatibility issues before fixing.

https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML/Runtime/Internal/Tools/CSharpApiGenerator.cs

[TomFinley]

Regarding "more verbose" @svick, I think we can have our cake and eat it too...

I don't consider the case where we're applying a transform to multiple columns simultaneously to be terribly important. I would therefore prefer that we get rid of the params constructors, and replace them with two constructors more-or-less mirroring the signature of the AddColumn methods, with this constructor convenience being reserved for the "one mapping" case.

More specifically: I would like to remove these two constructors with params, and replace them with two constructors that do not take params. One with signature string name and the other with string name, string source. I would also prefer for clarity reasons that the code path for the first constructor be empty and just call this(name, name), since that makes the actual behavior more clear to people that call the code. (For the same reason I would also prefer that the AddColumn overload just call the second one, since right now they're phrased as entirely separate codepaths, which is confusing.)

In that case the only thing that remains "inconvenient" is the multi-mapping case, which I believe is actually fairly rare.

An aside: using ValueTuple in this API was not the best choice; I have absolutely no idea which of the two is the source, and which is the name of the new column. Certainly having no documentation on the consturctor hardly helps matters, but even if we did write documentation just having the parameter names making clear what I'm assigning is attractive in its own right.

Regarding the .Add(...).Add(...) idea, whether objects that are mutating the object should return the object itself is an interesting idea. I might prefer that if we decide to go that route that we do so in a more wholesale fashion (we can since much of this is being autogenerated anyway), rather than for this one type of method specifically. (I think it's been brought up in other contexts, but I can't quite find them.)

[svick]

@TomFinley Your plan sounds good to me.

An aside: using ValueTuple in this API was not the best choice; I have absolutely no idea which of the two is the source, and which is the name of the new column. Certainly having no documentation on the consturctor hardly helps matters, but even if we did write documentation just having the parameter names making clear what I'm assigning is attractive in its own right.

Not that it makes any difference if the tuple constructor is going to be removed, but that could be improved by using named tuple:

public ColumnCopier(params (string name, string source)[] inputOutputColumns)

Regarding the .Add(...).Add(...) idea, whether objects that are mutating the object should return the object itself is an interesting idea.

If transforming multiple columns is not an important use case, then it probably doesn't make much sense to make the API more complicated to make it easier to use here.

[glebuk]

There are several ways to address it. The complexity comes from the fact that we have several different kind of transforms with regards to how columns are handled:

1. 1 -> 1: Once input becomes another output, such as Rename
2. Many -> 1: many inputs become one output, such as Concat
3. None -> 1: output with no inputs, such as Random Number Generator
4. 1 -> none : input with no output

Also there are variety of transforms, where every one of the styles above those above can repeat several times.

The challenge is how to compactly and non-ambigiously represent many potential possibilities. Perhaps we can always have a “columns” param that takes a class that can express precisely the right kind of column mapping.

[zeahmed]

I think this issue is no longer valid and has been partially addressed in the referenced PR #324. The old API is being deprecated.