Skip to content

KeyToValueMapping API is inconsistent with rest #2376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TomFinley opened this issue Feb 1, 2019 · 0 comments
Closed

KeyToValueMapping API is inconsistent with rest #2376

TomFinley opened this issue Feb 1, 2019 · 0 comments
Assignees
Labels
API Issues pertaining the friendly API

Comments

@TomFinley
Copy link
Contributor

Pursuant to #2064 and the work done in #2239, column mapping transforms are to have an output and input column. However, I notice something like this:

public static KeyToValueMappingEstimator MapKeyToValue(this TransformsCatalog.ConversionTransforms catalog, string inputColumnName)
=> new KeyToValueMappingEstimator(CatalogUtils.GetEnvironment(catalog), inputColumnName);

This has just one column specified, that is, there is no convenient way to map an input to an output column and have the mapping have a distinct names, without working through the params overload which is a bit less convenient. This is inconsistent with the direction summarized by @sfilipi in #2064 as well as some of our other APIs. For example, in this same file:

public static HashingEstimator Hash(this TransformsCatalog.ConversionTransforms catalog, string outputColumnName, string inputColumnName = null,
int hashBits = HashDefaults.HashBits, int invertHash = HashDefaults.InvertHash)

public static TypeConvertingEstimator ConvertType(this TransformsCatalog.ConversionTransforms catalog, string outputColumnName, string inputColumnName = null,
DataKind outputKind = ConvertDefaults.DefaultOutputKind)

This method should be similar to the others in terms of the string outputColumnName, string inputColumnName = null signature. Note that this mistake extends down into the associated estimator and transformer constructors as well, so it will have to be fixed there as well.

/cc @sfilipi

@TomFinley TomFinley added API Issues pertaining the friendly API answered and removed answered labels Feb 1, 2019
@najeeb-kazmi najeeb-kazmi self-assigned this Feb 5, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

2 participants