Repository navigation
ColumnCopier API is confusing #274
Description
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.
Activity
Ivanidzo4ka commentedon May 31, 2018
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:
svick commentedon May 31, 2018
@Ivanidzo4ka I'm looking at the source code in master:
machinelearning/src/Microsoft.ML/CSharpApi.cs
Line 8684 in 9d19d0e
machinelearning/src/Microsoft.ML/CSharpApi.cs
Line 8695 in 9d19d0e
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 commentedon May 31, 2018
@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 commentedon May 31, 2018
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 commentedon May 31, 2018
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
paramsconstructors, and replace them with two constructors more-or-less mirroring the signature of theAddColumnmethods, 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 signaturestring nameand the other withstring name, string source. I would also prefer for clarity reasons that the code path for the first constructor be empty and just callthis(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 theAddColumnoverload 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
ValueTuplein 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 commentedon Jun 1, 2018
@TomFinley Your plan sounds good to me.
Not that it makes any difference if the tuple constructor is going to be removed, but that could be improved by using named tuple:
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 commentedon Jun 5, 2018
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:
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 commentedon Jan 3, 2019
I think this issue is no longer valid and has been partially addressed in the referenced PR #324. The old API is being deprecated.
Printing additional metrics in CLI for binary classification (dotnet#274
code migration from machinelearning-automl to machinelearning repo fe…
Printing additional metrics in CLI for binary classification (dotnet#274
Added onnx export functionality for LpNormNormalizingTransformer (#4161)
Added Onnx export functionality to PCATransformer (#4188)