Skip to content

Estimator arguments should take output column name as first parameter, any inputs as subsequent parameters #2064

Closed

Description

@TomFinley

So, a somewhat embarrassing blunder to report... not quite sure how to say this.

Throughout our codebase, for years we've always specified the name of the output column of a transform first, then the source(s).

That's good. But when estimators were introduced, somehow, nearly all of them were introduced in the reverse order: nearly all of them specify the inputs, then the outputs. This was probably an unconscious mistake, but it's one with fairly wide consequences, since that mistaken pattern was copied again and again as people did the estimator conversion work, to the point where most (not all!) estimators are now inconsistent with the whole rest of the codebase!

This should be corrected: it is at least inconsistent, and even if not inconsistent actually rather obnoxious practically because specifying the name of the output first has practical benefits, and makes a lot more sense, since if you're specifying a transformation the most important information someone will want to know is what you're calling your output!

The Story Until a Few Months Ago

So, throughout our codebase, for years, it has been our practice that when specifying a transform, you specify the name of the output, then you specify the name of the input(s) (if any). The reason for this is practical: the outputs are usually the result of a well defined single calculation (the application of the transform), whereas what is taken as an "input" to a transform can have various meanings since it is sometimes a multivariate function in its inputs more often than in its outputs. (E.g., concatenate can have multiple inputs, text can have multiple inputs, MI feature selection takes multiple heterogeneous inputs, but all produce single output columns.)

This was our practice certainly when this code was a tool, as we see in the command line help string, specifying name then source.

[Argument(ArgumentType.Multiple | ArgumentType.Required, HelpText = "New column definition(s) (optional form: name:src)", ShortName = "col", SortOrder = 1)]
public Column[] Column;

This trend continued during our initial attempts at an API, as seen in this early discussion in PR 405, and as seen here:

https://github.com/zeahmed/machinelearning/blob/16f7883933b56f8fd86077bf0fd262b24374e9d0/src/Microsoft.ML.Data/Transforms/ConvertTransform.cs#L116

and here

https://github.com/zeahmed/machinelearning/blob/16f7883933b56f8fd86077bf0fd262b24374e9d0/src/Microsoft.ML.Data/Transforms/DropSlotsTransform.cs#L226

and numerous other places.

This is done for the practical reason that, when a transform produces an
output, what outputs it has are usually finite and well defined, whereas it can take multiple examles. The most conspicuous and widely used example of this is
the concatenation transform. Also included are things like the text featurizing transform, and other such things.

So far so good...

But Then...

Now, somehow, through some mechanism that wasn't quite clear to me, as IEstimator implementations are being created, someone commits a fateful mistake of reversing inputs and outputs. Suddenly instead of being typically parameterized as name and a sometimes optional source, we instead have the required input and a sometimes optional output. What a disaster! And, I did not catch it in review. An early example:

public TextTransform(IHostEnvironment env, IEnumerable<string> inputColumns, string outputColumn,
Action<Settings> advancedSettings = null)

Then, as people use this as a template for their own estimator conversion work, nearly all estimators copied this mistake, until practically all estimators had this problem. This includes much of the extensions.

public static ColumnCopyingEstimator CopyColumns(this TransformsCatalog catalog, params (string source, string name)[] columns)
=> new ColumnCopyingEstimator(CatalogUtils.GetEnvironment(catalog), columns);

Now then, the reason why I know any of this is that @stephentoub wrote and says, "hey, how come you have your column concatenation operation specify output then inputs? That's different from everywhere else! I know it's less convenient, but it's better to be consistent."

public static ColumnConcatenatingEstimator Concatenate(this TransformsCatalog catalog, string outputColumn, params string[] inputColumns)

And, after thinking that he must be very confused, since again, the pattern of name then source being very, very well defined throughout the codebase, I look and find he is absolutely correct, and a thoroughly massive blunder had somehow made it throughout the entire codebase under our very noses, including it must be said mine. :) :)

So, this is bad, but happily this was caught before v1. But, needless to say this must be fixed immediately.

Activity

  1. justinormont commented on Jan 8, 2019

    @justinormont
    Contributor

    Semi-related: We may also want to take a pass at ensuring the parameter names are consistent as I expect this to be difficult to correct after v1.0. I assume the work would be creating a large spread sheet of rows of components & columns of parameter names (and ensuring we don't create a new column needlessly).

  2. Ivanidzo4ka commented on Jan 8, 2019

    @Ivanidzo4ka
    Contributor

    I would like to disagree. (With idea what Output first, inputs later)

    The reason for this is practical: the outputs are usually the result of a well defined single calculation (the application of the transform), whereas what is taken as an "input" to a transform can have various meanings since it is sometimes a multivariate function in its inputs more often than in its outputs.

    I've tend to believe what reason behind name-> source is command line which force you to express yourself in kinda whiteboard way so it's somewhat natural to write y = func(x1,x2,x3) where you specify y first and then you specify rest.

    For programmatic API in C# I found idea to write output parameter first, and then everything else, is bizarre and against nature. I look on transformers as some functions on top of data.
    https://docs.microsoft.com/en-us/dotnet/api/system.func-6?view=netframework-4.7.2
    Func<in T1,in T2,in T3,in T4,in T5,out TResult>
    I don't see this idiom of putting output first in the way how C# views functions in general, I would prefer to have current order of most estimators to remain present.

  3. TomFinley commented on Jan 8, 2019

    @TomFinley
    ContributorAuthor

    For programmatic API in C# I found idea to write output parameter first, and then everything else, is bizarre and against nature. I look on transformers as some functions on top of data.
    https://docs.microsoft.com/en-us/dotnet/api/system.func-6?view=netframework-4.7.2
    Func<in T1,in T2,in T3,in T4,in T5,out TResult>
    I don't see this idiom of putting output first in the way how C# views functions in general, I would prefer to have current order of most estimators to remain present.

    Hi @Ivanidzo4ka ! Thanks for your feedback. I am not sure exactly what you mean. Are we talking about the same language? Indeed, let's take your precise example. The signature of that is very clear.

    public delegate TResult Func<in T1,in T2,in T3,in T4,in T5,out TResult>(T1 arg1, T2 arg2, T3 arg3, T4 arg4, T5 arg5);

    I see a TResult first, then all these T1 and so on defined after that in the actual language syntax (whatever they chose to do with the generic parameters). I'm not trying to put you on the spot or anything, but I mean, it's right in the link you gave. Then I see the inputs. Indeed, every function declaration is like this in C#. Return type is first. Variable assignment is the same. In C#, Java, C++, C, Python, Ruby, it is something like this:

    int a = b + 2;

    not this

    b + 2 => int a;
    

    Indeed, most languages I can think of when assigning variables... we have F# with its let a = b + 2, other similar things elsewhere. I mean, I guess back when I was a kid I used HyperTalk which had syntax like put 5 * 4 into theResult, but I can't say too many other examples come to mind.

    This of course assuming I accept the idea that we have to be analogous to a programming language, which I'm not sure I do, but if I did, it certainly appears at first glance to be utterly disastrous to your case. But in any case, generally I think the most important information should generally go first. What I want to know out of every transform is "what are you making, and how are you making it," not "how are you making something, and what is it called?"

    This in addition to the advantages already enumerated originally, the consistency argument, etc. The fact that when transforms take multiple inputs many times those auxiliary helper inputs are optional (so should have null defaults), but the name of the output never is. So, I'm not sure what you're talking about. Without assigning blame, and without belaboring the point, this was clearly a blunder. Massive error. Let's fix it please.

    Semi-related: We may also want to take a pass at ensuring the parameter names are consistent as I expect this to be difficult to correct after v1.0. I assume the work would be creating a large spread sheet of rows of components & columns of parameter names (and ensuring we don't create a new column needlessly).

    @justinormont that's a good point actually, even for this. (I mean, you're right in the broader sense, but just going to stay on topic.) Not only did we screw up the ordering of the column names, we even screwed up their names. For years even with one-to-one transforms these have been named name (for the name of the output) followed by source (For the name of the single source, assuming there was a single source, which there often was not).

    We have changed this to input and output, which is just confusing, because the "input" and "output" in the case of an IEstimator and ITransformer is already a very well defined intuitive concept. The input to an estimator is an IDataView provided to fit, and the output is the ITransformer. The input to every ITransformer is the data to be transformed, the output is the transformed data. To reuse the same term for this other thing when we already had a perfectly servicable name is bad.

    So in addition to changing the order: these parameter should be named, as they are literally everywhere else, name and source, for the same of sanity and consistency.

  4. stephentoub commented on Jan 9, 2019

    @stephentoub
    Member

    Indeed, every function declaration is like this in C#

    It's not about return values, it's about parameters and the order used in .NET APIs in general when an API has some kind of source parameter and some kind of destination parameter.

    For example:

  5. TomFinley commented on Jan 9, 2019

    @TomFinley
    ContributorAuthor

    But this is my point. It's not always about a "source" and a "destination." There is almost always a destination, but the "source" is sometimes indeterminate, multiple, etc. We did not settle on this convention by accident.

    You'll note also that these are all about filling in values in an already allocated buffer. This is not that. It's more like, "we're declaring this new slot named name and please fill it in with an operation parameterized in the following way with many parameters, some of which may be source columns from which values are created."

  6. Ivanidzo4ka commented on Jan 9, 2019

    @Ivanidzo4ka
    Contributor

    but the "source" is sometimes indeterminate, multiple, etc.

    This is why I try to bring Func as example, but I guess I wasn't clear.
    Func is exact same case, right? You have multiple inputs and one output, but if you look all of them defined as
    Func<in T1,out TResult>
    Func<in T1, in T2, out TResult>
    and not
    Func<out TResult, in T1, in T2>
    And in general I found idea of Func(destination, source1, source2,source etc) counter intuitive. And I see same words in mail which spark this issue. Why we need to change all other code in our codebase instead of having consistent clean pattern of Input first, output second?

  7. TomFinley commented on Jan 9, 2019

    @TomFinley
    ContributorAuthor

    but the "source" is sometimes indeterminate, multiple, etc.

    This is why I try to bring Func as example, but I guess I wasn't clear.
    Func is exact same case, right? You have multiple inputs and one output, but if you look all of them defined as
    Func<in T1,out TResult>
    Func<in T1, in T2, out TResult>
    and not
    Func<out TResult, in T1, in T2>

    Thank you @Ivanidzo4ka for trying to explain to me again. I think I do understand the example, I am trying to explain gently that it points in a different direction than you suppose. I'll go over a separate point from my original one, in case it helps illustrate why.

    Let's actually look at a few of these extension methods, in particular, the string input, string output ones that you claim you are doing to be consistent with this more "intuitive" understanding based on Func<>. I will do so by searching the codebase for the string (this TransformsCatalog, in case you want to repeat the experiment, then looking through them, and finding the ones with this string inputColumn, string outputColumn pattern, or something similar.

    Since this involves an enumeration and examination, this response will be somewhat long, I'm sorry to say. I titled the examples so they're easy to browse, and put my conclusion at the thing titled "Conclusion" more towards the end.

    Simple Example 1

    The first one I see is Hash. It is this.

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

    I do not see the input parameters, followed by an output. Instead, I see one input paramter, followed by the output, followed by two more input parameters.

    Simple Example 2

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

    One input parameter, the output parameter, followed by one more input parameter.

    Complex Example 1

    Actually, now that I'm looking, there are a few more here that are worthy of inspection in this file; despite not directly filling my pattern, they indirectly do if we look at them. They also indirectly illustrate the practical reasons why we years ago arrived at the pattern that we did. These are, if anything, even more confused, since they have neither input nor output first!

    This is one. You will see we have the column definition first, which happens to be a ColumnInfo. It is parameterized in this way.

    public ColumnInfo(string input, string output = null,
    int maxNumTerms = ValueToKeyMappingEstimator.Defaults.MaxNumTerms,
    SortOrder sort = ValueToKeyMappingEstimator.Defaults.Sort,
    string[] term = null,
    bool textKeyValues = false

    So this again has the pattern of input, followed by output, followed by four more inputs. The same confused mixing.

    But let's return again then to the original linked code... we have more parameters. What are they? It is the name of the input file. Followed by the name of the input column from that file, followed by how to load this file. So we have more input "columns" calculated. So not only are we drawing an artificial distinction between the input column and other inputs to the estimator, we are not even being consistent with the input columns themselves.

    public static ValueToKeyMappingEstimator MapValueToKey(this TransformsCatalog.ConversionTransforms catalog,
    ValueToKeyMappingTransformer.ColumnInfo[] columns,
    string file = null,

    Complex Example 2

    public static ValueMappingEstimator<TInputType, TOutputType> ValueMap<TInputType, TOutputType>(
    this TransformsCatalog.ConversionTransforms catalog,
    IEnumerable<TInputType> keys,
    IEnumerable<TOutputType> values,
    params (string source, string name)[] columns)

    Here we've arrived at something else. This actually fulfills the pattern that you claimed you wanted to fulfill, of the output is actually specified last!! Let me ask you this. Do you like it? 😄 Is it consistent with anything? Would we want anything to be consistent with it?

    Simple Example 3

    public static ColumnCopyingEstimator CopyColumns(this TransformsCatalog catalog, string inputColumn, string outputColumn)

    This one actually fills your pattern, but only by virtue of having no parameterization except the name of the input, which is far from typical.

    There are a few more in here, but they're somewhat similar and confused (keep and drop which are kind of special in the sense that they describe only inputs, or only outputs, depending on how you look at it. Also actually concatenate, which is one of the few conversions that actually wound up being correct, and which sparked this conversation.)

    Simple Examples Batch 4

    Next up, normalizers. Only one here that directly fulfills the pattern.

    string inputName,
    string outputName = null,
    NormalizingEstimator.NormalizerMode mode = NormalizingEstimator.NormalizerMode.MinMax)

    Same confused pattern. Input, output, input.

    public static TextFeaturizingEstimator FeaturizeText(this TransformsCatalog.TextTransforms catalog,
    IEnumerable<string> inputColumns,
    string outputColumn,
    Action<TextFeaturizingEstimator.Settings> advancedSettings = null)

    Ooo. Inputs (as IEnumerable), output, more input (or more like a collection of input).

    public static TokenizingByCharactersEstimator TokenizeCharacters(this TransformsCatalog.TextTransforms catalog,
    string inputColumn,
    string outputColumn = null,
    bool useMarkerCharacters = CharTokenizingDefaults.UseMarkerCharacters)

    Same confused pattern. Input output input.

    Conclusion

    I hope you don't mind, I had intended to keep going, but, it's the kids' bedtime, I think by this point, it's getting fairly redundant, and I'm starting to belabor the point. So this is what you said:

    And in general I found idea of Func(destination, source1, source2,source etc) counter intuitive.

    That's fine. But if I were to actually put the destination last, past all other things we could reasonably call inputs, then the result would not be what we really want, since many of these "functional inputs" have default values, etc. You seem to favor neither approach. Instead, what you favor, to judge from your vigorous defense, is something chimerical: Func<TIn1, TResult, TIn2, TIn3> or Func<TIn, TResult, TIn2>. I mean, we could continue -- maybe we want to argue that some inputs go before the result, some inputs go after, but, let's please not. It's been a long enough day already.

    And I see same words in mail which spark this issue. Why we need to change all other code in our codebase instead of having consistent clean pattern of Input first, output second?

    As we see in the original text of the issue, this is somewhat reversed. This new pattern introduced a few months ago is inconsistent with our longstanding practices. The mail is informative in pointing out the discrepancy and other issues (which we must fix), but we must decide how. And, sometimes (in fact, always) specific suggestions made spur the moment have to be carefully considered if they are in fact wise. That this blunder got repeated so many times that it somehow got established as "the way" by some people is unfortunate, but, again, it must be fixed.

    I hope it's obvious that I really value your input @Ivanidzo4ka and @stephentoub. You are enormously helpful. I generally find myself in agreement with you on most things, and I hope one disagreement doesn't put you off. But, unpleasant as I find it to do so, I can't agree in this case. I'm sorry.

  8. self-assigned this
    on Jan 9, 2019
  9. Ivanidzo4ka commented on Jan 10, 2019

    @Ivanidzo4ka
    Contributor

    If you look on all that cases as input, output, input, I completely agree with you, it's awful way to design Api. But I view that as input, output, params, and in that case, I find it a nice and convenient pattern to use in code.

    I've check examples you linked and all of them follow input, output, params (where params is optional) except Complex Example 2 which looks awful.

    I've look into links provided by @stephentoub and they either use same pattern input, output, optional params or input, params for inputs, output, params for output. I have troubles to find methods which would accept output first, and then input, but that's probably my lack of searching skills.

    You seem to favor neither approach. Instead, what you favor, to judge from your vigorous defense, is something chimerical: Func<TIn1, TResult, TIn2, TIn3> or Func<TIn, TResult, TIn2>.

    I was just try to point out what I don't see pattern where people specify output result first.

    This new pattern introduced a few months ago is inconsistent with our longstanding practices. The mail is informative in pointing out the discrepancy and other issues (which we must fix), but we must decide how.

    Again, let me disagree, we don't have longstanding practices of developing good public API. I agree we should have consistent API, I just disagree with output first, everything is later approach.

    But in any case, generally I think the most important information should generally go first. What I want to know out of every transform is "what are you making, and how are you making it," not "how are you making something, and what is it called?"

    I hope I correctly depicted your main reasoning behind having output column first and input columns second. You find it more practical and this is why you want output to be first.
    I think it's a subjective statement, and I struggle to find any examples of good C# code where that approach was implemented. (That's again subjective of me, to say good C# code)

    So basically we are in holy war state, Little endians vs big endians.
    How civilized humans decide such conflicts?

  10. TomFinley commented on Jan 10, 2019

    @TomFinley
    ContributorAuthor

    If you look on all that cases as input, output, input, I completely agree with you, it's awful way to design Api. But I view that as input, output, params, and in that case, I find it a nice and convenient pattern to use in code.

    So in your first version of the argument there are "inputs" and "outputs." Then I point out it is not so. Now there are "inputs" and "outputs" and "parameters." So let me ask, what is an "input?" What is a "parameter?" Can you define it for me, precisely and clearly in a way I can apply and explain in all cases? I repeat again the lesson taught in my prior "Complex Example 1"...

    public static ValueToKeyMappingEstimator MapValueToKey(this TransformsCatalog.ConversionTransforms catalog,
    ValueToKeyMappingTransformer.ColumnInfo[] columns,
    string file = null,
    string termsColumn = null,
    IComponentFactory<IMultiStreamSource, IDataLoader> loaderFactory = null)

    So, in this case, the "inputs" vs. "parameters," I guess, are that the things defining the terms with its file and column names are "parameters." How is this decided? What is the correct split then? Is the termsColumn an "input" or a "parameter?" It looks kind of like an "input" to me. So should it go first? How about the file name? It seems odd that this should be separated, so that is also an input? What about the loader factory? An input, since it should probably not be separated from those?

    You see, I hope, why I don't find this idea of being "inputs" and "parameters" convincing.

    It is true certainly that sometimes the API must differ from the tool that's been around for years. I have argued this myself when it makes sense on many occasions. An API is a different environment after all, some problems have different natural solutions given that we're not writing a GUI or command line tool. But usually, you'll note that when we have a disparity, we actually provide reasons. In this case, it is unclear to me that the reasons why we did things they way we've done for years have changed. Certainly I do not find merely mentioning that we sometimes do it sufficient justification.

    Also, not to put too fine a point on it, but I did explicitly already talk about how futile it was to try to draw a sharp line between "some inputs go before the result, some inputs go after."

    I've look into links provided by @stephentoub ...
    I was just try to point out what I don't see pattern where people specify output result first.

    Putting stuff into an already established (and, yes, named) buffer was not an analogy I found compelling. It doesn't reflect what's going on at all. Far closer is creating something new and giving it a name. In such APIs like that, I almost always see the name is the created thing is usually before anything related to its value, or parameterization.

    So: you're not providing an output. The output does not exist yet. You're creating it, right then. You're naming it. In any such API I can find where you're naming something and then providing how it is defined, when naming a new item and defining it, usually it is the name of the new item that comes first, then the content.

    I mean, even in the most basic data structures. Most people call them "key-value" and not "value-key" stores for a reason. Right? Plus of course I already of course pointed out that every C derived language puts the name before the value as part of its basic syntax.

    (Please, I know that I write somewhat voluminously, but, every last thing you're bringing up I've already covered. I know, I know, "tl;dr", but, please do. I'm spending so much time on this because I take it seriously.)

    So basically we are in holy war state, Little endians vs big endians.
    How civilized humans decide such conflicts?

    First you try to avoid the conflict in the first place, by trying to reach consensus. This happens of course 99% of the time. In the remaining 1% where someone is insistent in the face of all argument, ultimately, in this case as in all cases we must make a single decision, since the code cannot be in superposition. A choice must be made. So, I try to be as gentle and polite as I can, explaining the decision, continue to try to reach consensus. (Again, I invite you to read my arguments.) If it still doesn't work, well... we obviously can't continue forever, so the hard decision is made.

    We've done this in some other occasions. With the entry-point based API, the "predicting model" of the v0.1 API, the ideas that mutable estimators are a good idea. This one thing isn't the first, and I'm sure won't be the last. I always find it deeply unpleasant, but, what can you do.

  11. glebuk commented on Jan 16, 2019

    @glebuk
    Contributor

    Leaving the order of arguments aside, let's discuss column argument names. I've always found the current default of source/name to be quite confusing.

    Let's look at the current state.

       ColumnCopyingEstimator ( string input, string output) 
       ColumnCopyingEstimator (params (string source, string name)[] columns)
       NormalizingEstimator Normalize(… params (string input, string output)[] columns)
       NormalizingEstimator Normalize(...params NormalizingEstimator.ColumnBase[] columns)
         /* where the ctor is: */      ColumnBase(string input, string output)

    We have at least three way to declare inputs and outputs: via strings, via tuples and via Column classes. We should come up with a solution that is readable and consistent between all those usages.

    However, what are the alternatives?
    There at least the following options to consider, where the same name will be used in all possible ways to define columns:
    1. {name, source} - the "name" is ambigious and assymetrical with "source"
    2. {input, output} - concise and clear, but can be confused for IDV input/output - but is this an issue in this case? There are no input/output IDVs as constructor args anywhere...
    3. {source, destination}
    4. {inputColumn, outputColumn} - most clear but most verbose

    Thoughts?
    My personal ordered preference is: # 3, # 2, # 4, # 1.

  12. TomFinley commented on Jan 16, 2019

    @TomFinley
    ContributorAuthor

    Let's look at the current state.

       ColumnCopyingEstimator ( string input, string output) 
       ColumnCopyingEstimator (params (string source, string name)[] columns)
       NormalizingEstimator Normalize(… params (string input, string output)[] columns)
       NormalizingEstimator Normalize(...params NormalizingEstimator.ColumnBase[] columns)
         /* where the ctor is: */      ColumnBase(string input, string output)

    We have at least three way to declare inputs and outputs: via strings, via tuples and via Column classes. We should come up with a solution that is readable and consistent between all those usages.

    You will note that all of these things are, each last one of them., estimators. The fact that estimators diverged from our already established pattern is the subject of this issue. We already have a solution ready to hand: make this new IEstimator idiom introduced a few months ago consistent with what we have done for years.

    However, what are the alternatives?
    There at least the following options to consider, where the same name will be used in all possible ways to define columns:
    1. {name, source} - the "name" is ambigious and assymetrical with "source"
    2. {input, output} - concise and clear, but can be confused for IDV input/output - but is this an issue in this case? There are no input/output IDVs as args anywhere...

    Again, this discussion concerns IEstimators primarily. I will quote it in its entirety:

    public interface IEstimator<out TTransformer>
    where TTransformer : ITransformer
    {
    /// <summary>
    /// Train and return a transformer.
    /// </summary>
    TTransformer Fit(IDataView input);
    /// <summary>
    /// Schema propagation for estimators.
    /// Returns the output schema shape of the estimator, if the input schema shape is like the one provided.
    /// </summary>
    SchemaShape GetOutputSchema(SchemaShape inputSchema);
    }

    You will note two methods. One for schema propagation prior to fitting and one for actually fitting. One takes the input's schema (or more precisely, the more provisional parallel structure, a "schema shape") to perform preliminary schema validation and propagation -- that input named inputSchema, I hope you observe. The second is fitting and returning a transform given data. That data is given the name, again, input. You will note a striking similarity in the definition of ITransformer. It is in the same file.

    I mean, It's right there in the interface signature of the structure that is the subject of this discussion, right?

    Thoughts?
    My personal ordered preference is: # 3, # 2, # 4, # 1.

    Am I to understand that 3 is your first choice and 1 is your most disfavored choice?

    You would favor renaming what we currently call "name" to "destination." In fact that one change alone would shift your opinion from liking the solution we are pursuing least to liking it best. In fact that would be absolutely perfectly clear in your head.

    Let's be clear. These estimators are, in some sense, creating new columns. Everywhere* that we talk about columns, what is the primary identifier for a column? Always "name." In Schema.Column, in Schema when accessing columns, in loaders when specifying columns, the array data view builders, the attributes for creating a data view out of a C# type, the dataview design principles that I hope we're all very familiar with...

    Let's not even talk about our code, but all similar code like SQL and data frames and Parquet files. What do they call the string identifier they use to reference a column? The column's name. Not only are we internally consistent, but we are consistent with longstanding usage of this sort of structure.

    But suddenly, now, after years of calling this thing "name" everywhere, we have suddenly had an epiphany that in estimators specifically (and only there, apparently) calling it name is now incorrect, and now it should be called destination. How can you possibly account for this disparity with our usage of this structure? Why would we introduce such an inconsistency?

    You also asked for thoughts, but I will express them privately.

  13. glebuk commented on Jan 16, 2019

    @glebuk
    Contributor

    We have several places where the concepts of "input column names" and "output column names " are used.
    Namely:

       // simple-typed estimator ctor args: 
       EstimatorFactoryMethodFoo(string name, string source) 
       // tuple-typed ctor args:  
       EstimatorFactoryMethodFoo (string name, string source)[] columns)
       // column info-typed ctor args:  
       EstimatorFactoryMethodFoo (Foo.ColumnBase[] columns)
            // where the Column type has fields 
            class ColumnBase  { string Name; string Source; }
       // readers:
       CreateTextReader(... TextLoader.Column[] columns)
            // where Column ctor as defined as
            public Column(string name, DataKind? type, int index)
       // and a few others such as command line args and entrypoints

    If we want the variable/field naming consistent among all of those usages, we will have to stay with the original naming convention of the {name, source}, at the expense of fluency in case of simple-type ctor usage.

    @KrzysztofCwalina @eerhardt @terrajobst - can you please weigh in?

  14. KrzysztofCwalina commented on Jan 17, 2019

    @KrzysztofCwalina
    Member

    In new .NET APIs (Span transformations) we use source/destination. I think I started to use the names (in the transformation APIs) because in many existing APIs where we actually use input/output (or in/out) the direction of data flow is actually not very intuitive. For example, you might be surprised to discover that Console.In is actually something that returns TextReader that the callers read (duh).

  15. 4 remaining items

  16. eerhardt commented on Jan 22, 2019

    @eerhardt
    Member

    We have dissatisfaction expressed, no alternative is being proposed.

    I'm trying to understand all the issues being raised before proposing an alternative.

    What am I supposed to do with that?

    I would expect someone to read and understand the feedback, and think about any merits the feedback has.

    Because elsewhere when naming new columns that is called it's name and that argument is always first, as far as I see.

    I really want to understand why you are so insistent on the name of the output/destination column parameter be simply name, and the name of the input/source column parameter be source. You say that's the way it was everywhere - but that is regardless of the context in which it is being used?

    I totally understand when you have a class/struct that is named something something Column, and it has a set of properties, like in the OneToOneColumn and ManyToOneColumn classes:

    public abstract class ManyToOneColumn
    {
    [Argument(ArgumentType.AtMostOnce, HelpText = "Name of the new column", ShortName = "name")]
    public string Name;
    [Argument(ArgumentType.Multiple, HelpText = "Name of the source column", ShortName = "src")]
    public string[] Source;

    This makes sense because of the context in which the properties are being used - they are in a Column class. The usage of the word name is obvious - it refers to the name of the column.

    However, I am not convinced that it is obvious, or even intuitive, that when I am creating an Estimator and asked to provide a name that I am really being asked for the name of the output column. I am no longer in the context of a Column.

    Is the only reason you are rejecting having the parameter names being destinationColumn and sourceColumn(s) (or output/input) because you feel it is inconsistent with the properties Name and Source on classes like the ManyToOneColumn class above? Or are there other reasons?

  17. Zruty0 commented on Jan 23, 2019

    @Zruty0
    Contributor

    OK, I guess I need to weigh in as well. First of all, definitions:

    • inputs - column names that are consumed by the estimator.
    • outputs - column names that are produced by the fitted transformer.
    • params - all other constructor parameters, except the above.

    @TomFinley is right: it was my suggestion to change to the current scheme ((string inputColumn, string outputColumn = null, all_other_params)), because I found it very 'natural' for the majority case.

    The majority case is that we have 'one to one column mapping estimators'. So, in the ideal world, we would have the only factory method to be mlContext.FooEstimator(string inputColumn, string outputColumn = null). Uncontroversial so far?

    Even in this simple case, there is complexity:

    • The mapping often has parameters. My suggestion was to have "input column, output column, then other params":
    mlContext.FooEstimator(string inputColumn, string outputColumn = null, float param1 = 0, string param2 = null);

    This is very similar to common .NET APIs (like File.Copy

    • For performance reasons, we often 'bundle' multiple per-column mappings into one estimator. This gives rise to the (string inputColumn, string outputColumn)[] pattern.

    • Both of the above can be combined: additional mapping parameters can be either shared across all column mappings, or be specified per-mapping.

    So, for regular rank-and-file 'one-to-one mapping', we already have a matrix of possible factory methods:

    . No extra params Has extra params
    One column pair string inputColumn, string outputColumn = null string inputColumn, string outputColumn = null, otherParams
    Multiple column pairs (string inputColumn, string outputColumn)[] ColumnInfo[] columns

    In the hindsight, I think this 4-way matrix was not necessary. I would suggest to trim down to only 2 options:

    . No extra params Has extra params
    One column pair string inputColumn, string outputColumn = null string inputColumn, string outputColumn = null, otherParams
    Multiple column pairs ColumnInfo[] columns ColumnInfo[] columns

    Arguably, maybe we can take it even further? We could declare that the ONLY special treatment applies to SINGLE column pair with NO extra params:

    . No extra params Has extra params
    One column pair string inputColumn, string outputColumn = null params ColumnInfo[] columns
    Multiple column pairs params ColumnInfo[] columns params ColumnInfo[] columns

    In any case, let us move from the simple majority case to the exceptions.
    I can identify three kinds of exceptions:

    • Trainers. They generally tend to follow their own naming pattern, with only 'input columns' being specified, and 'output columns' being completely fixed:
    mlContext.BinaryClassification.Trainers.Foo(string featureColumn = "Features", string labelColumn = "Label", string weightColumn = null, float customParam1 = 0, string customParam2 = null)

    I would argue that in this case we can leave things be. Trainers tend to be both unique and fairly uniform with column mapping, and they also all reside in mlContext.Task.Trainers, so there is enough consistensy as it is.

    • Many-to-one estimators. Those that take more than one input to form the output column. Most notably, this is Concat, but there may be others that I don't remember now.

    In this case, it looks hard to define signature consistent with the above (where we specify inputs, then outputs, then other params). However, we can still do it:

    mlContext.ManyToOneEstimator(string[] inputColumns, string outputColumn, extraParams);

    Not exactly identical to the above, but the pattern is the same. More than one input column - good. Not nullable output - also good (since we no longer assume that output column name defaults to the input name).

    Again, in case of any multiple-mapping scenarios we can back off to the 'standard' column-object approach:

    mlContext.ManyToOneEstimator(params ColumnInfo[] columns);
    • Many-to-many estimators. These take multiple input columns to produce multiple output columns.
      There are weird cases like TextFeaturizer that may produce 1 or 2 output columns, controlled by a boolean flag, and there's the CustomMappingEstimator that doesn't take inputs and outputs as args (and uses schema comprehension to define the mapping).

    I believe it's tricky to have a consistent interface for all these cases, but I think we can strive to adopt some form of unification.
    <0 or more input columns> <0 or more output columns> <all other parameters>
    Essentially, we could just prohibit output columns to appear before inputs, and parameters to appear before inputs or outputs.

    Again, when applicable, we can back off to the good old params ColumnInfo[] columns.

  18. Zruty0 commented on Jan 23, 2019

    @Zruty0
    Contributor

    So, TL;DR version of the above is: let's put all inputs first, then all outputs, then all other params.

    Now let me go over Tom's examples and try to come up with a unified conversion.

    Simple 1

     public static HashingEstimator Hash(this TransformsCatalog.ConversionTransforms catalog, string inputColumn, string outputColumn = null, 
         int hashBits = HashDefaults.HashBits, int invertHash = HashDefaults.InvertHash) 
    • Recommendation: leave as is.

    Simple 2

    public static KeyToValueMappingEstimator MapKeyToValue(this TransformsCatalog.ConversionTransforms catalog, string inputColumn)

    This looks just like a bug to me. Should have the output column.

    • Recommendation:
    public static KeyToValueMappingEstimator MapKeyToValue(this TransformsCatalog.ConversionTransforms catalog, string inputColumn, string outputColumn = null)

    Complex 1
    This is the params ColumnInfo[] version, with column being created as

                public ColumnInfo(string input, string output = null,
                    int maxNumTerms = ValueToKeyMappingEstimator.Defaults.MaxNumTerms,
                    SortOrder sort = ValueToKeyMappingEstimator.Defaults.Sort,
                    string[] term = null,
                    bool textKeyValues = false)
    • Recommendation: leave as is. Maybe clean up the actual constructor to be 'terms' instead of 'term', but that's all.

    Complex 2

            public static ValueMappingEstimator<TInputType, TOutputType> ValueMap<TInputType, TOutputType>(
                this TransformsCatalog.ConversionTransforms catalog,
                IEnumerable<TInputType> keys,
                IEnumerable<TOutputType> values,
                params (string source, string name)[] columns)

    This looks really like an oddball: parameters before everything. I am not sure what the thinking was behind it.

    • Recommendation: split into 2 methods.
            public static ValueMappingEstimator<TInputType, TOutputType> ValueMap<TInputType, TOutputType>(
                this TransformsCatalog.ConversionTransforms catalog,
                string inputColumn, 
                string outputColumn,
                IEnumerable<TInputType> keys,
                IEnumerable<TOutputType> values);
    
            public static ValueMappingEstimator<TInputType, TOutputType> ValueMap<TInputType, TOutputType>(
                this TransformsCatalog.ConversionTransforms catalog,
                params MappingColumnInfo<TInputType, TOutputType>[] columns); // for multi-pair mapping.

    Here, we are not 100% consistent (output column is not nullable, although we'd like it to be). This is because the params are required, there is no default behavior. I think this is small price to pay for the streamlined order of 'inputs, outputs, params'.

    Simple 3

    public static ColumnCopyingEstimator CopyColumns(this TransformsCatalog catalog, string inputColumn, string outputColumn)
    • Recommendation: leave as is. Output column defaulting to input makes no sense, so let's make it required.

    Text

     public static TextFeaturizingEstimator FeaturizeText(this TransformsCatalog.TextTransforms catalog, 
         IEnumerable<string> inputColumns, 
         string outputColumn, 
         Action<TextFeaturizingEstimator.Settings> advancedSettings = null) 
    • Recommendation: leave as is. Actually, I think the jury is still out on the Action<Settings> advancedSettings, but this is a separate topic. Also, I am not sure text featurizer ACTUALLY benefits from being a many-to-one transformer all that much: maybe we could just split this method into two, one for string inputColumn, string outputColumn = null, otherParams, and the other one to be params ColumnInfo[] columns.

    Keep, Drop
    These are the only estimators that drop columns, but we can still make them mostly fit the pattern:

    Keep(params string[] outputColumns);
    Keep(IEnumerable<string> outputColumns, bool keepHidden);
    Drop(params string[] inputColumns);
    Drop(IEnumerable<string> inputColumns, bool keepHidden);

    Concat
    Lastly, concat, I think, will suffer the most in terms of verbosity. But having params string[] inputColumns, while concise, goes against the proposed pattern, so I'm OK with this.

    Concat(IEnumerable<string>inputColumns, string outputColumn);
    Concat(params ColumnInfo[] columns);
  19. Zruty0 commented on Jan 23, 2019

    @Zruty0
    Contributor

    Reading through the rest of the comments.

    1. I second @Ivanidzo4ka and @glebuk 's sentiment that there the 'destination before source' pattern is really bizarre and we should not have it.

    2. I agree with @KrzysztofCwalina 's suggestion of input/output -> source/destination. sourceColumn and destColumn sound like a good pair to me (consistent with File.Copy again).

  20. sfilipi commented on Jan 23, 2019

    @sfilipi
    Member

    Moving my old comment from the PR to the issue, so all discussion is in one place, and adding a note about defaults.

    1- I also prefer the sourceColumn, destinationColumn (anything but the word "name" for the generated column). "name" as an API parameter is not meaningful. I agree that we are not going to be consistent with out command line usage, and entry point usage, but i think that is worth the user experience.

    2- After having worked through the changes adapting the new proposal, my biggest problem is that we are assigning default values to the source column, rather than making it mandatory.
    So by calling this:

    ml.Transforms.Normalize("Induced");

    on the api with the new signature:

    public static NormalizingEstimator Normalize(this TransformsCatalog catalog,
              string name,
              string source = null,
               NormalizingEstimator.NormalizerMode mode = NormalizingEstimator.NormalizerMode.MinMax)
    
    

    The users need to know that there exists an input column with the name "Induced", and that if they assign that string to the output/name column param, everything will be fine.

  21. TomFinley commented on Jan 23, 2019

    @TomFinley
    ContributorAuthor

    Here, we are not 100% consistent (output column is not nullable, although we'd like it to be). This is because the params are required, there is no default behavior. I think this is small price to pay for the streamlined order of 'inputs, outputs, params'.

    Let's stop right there, because this is the root of where we've gone down different paths, and if we can't agree on this, we can't agree on anything. This is the root of why I consider your scheme dead wrong. You gloss over it, but this is the point of why we decided a while ago to go in the opposite direction back when @zeahmed was writing the convenience constructors for transforms.

    Look at this discussion. People have the idea of "input" and "output" so baked into their heads that when I asked them to consider alternate cases where this wasn't quite so simple, most of them simply could not continue the discussion. How many weeks passed before someone besides me even bothered to mention this other case? That's how basic and fundamental this idea is, that people didn't even think about alternates.

    That in mind, let's now think about what you've just said: You've observed, as I did, that as soon as this "other required arguments" thing pops up, the scheme falls to pieces. We're talking about inconsistency of the most basic part of estimators. And your response to such a fundamental inconsistency is "small price to pay?" Really? No, no no no... that is a huge price to pay. This is literally the most basic thing we have to get right. My premise is that we must be consistent there, and if our scheme does not allow us to be consistent, then it is discarded. Hence this issue, where I tried to undo all this, since I view it as a gigantic blunder.

    But, you know what, that's all fine. Since everyone agrees this pattern is great, maybe I'm wrong. I disagree and think you're committing an error that will haunt this API, but it is simply impossible for me to continue this discussion, however strongly I think you're all making a huge blunder. So, do what you like here.

    Just, some of you, if you're at least willing to consider the possibility that this inconsistency represents a fatal flaw... for your sake at least, I'll retrace my steps that I took with @zeahmed this past summer. If you continue on this path, what you'll I hope realize is that it is useless to talk about "input" then "output" parameters and say one is necessarily first then second, but due to the basic syntax of C# there is only required and unrequired parameters. If we want either one of the "input" or "output" [sic] column names to be non-required, then you will find based on an examination of actual cases that it is the "inputs" that tend to be most polymorphic w.r.t. their behavior (in the sense that they are sometimes absent, optional, or whatever), and so most naturally group them among unrequired. Then among the required you will arrive at the scheme of the "output" column name being first, followed by any other required parameters (if any). But, you haven't, and that's fine. I'm only one person and I can't fight against every thing.

  22. markusweimer commented on Jan 23, 2019

    @markusweimer

    Regarding the ordering of inputs and outputs, I believe there is other important related work to consider: SQL. There, the output is specified before the input: SELECT something FROM somewhere. I believe this is precisely because the output is "new", and usually short, while the input can be hideously complex with structure in its own right.

    @interesaaat could be in a position to (dis)proof this perspective.

  23. eerhardt commented on Jan 23, 2019

    @eerhardt
    Member

    Here, we are not 100% consistent (output column is not nullable, although we'd like it to be)

    In general, the proposal of having the output column being nullable is a bit strange to me. Instead of having a pattern:

    FooEstimator(string inputColumn, string outputColumn = null, ...)

    What if we split these methods into two overloads:

    FooEstimator(string column, ...)
    FooEstimator(string inputColumn, string outputColumn, ...)

    If we have a 1-1 transform, and we want the ease of just specifying a single column name, and not the same name for both the input and output columns, then use overloads instead of nullable/default parameters for one of the columns.

    Would that make everything consistent?

  24. interesaaat commented on Jan 23, 2019

    @interesaaat

    Replying to Markus: you are right, the output goes last and I think that this is because it ease composability. SQL queries (as well as Datalog for that matter) are in fact composable, and it is much easier to get the output schema if is specified as first than if it is instead baked into the query (or last). Nevertheless, when you write queries, we always start from the input schema (it is difficult to derive the output schema without knowing what you get as input). So in practice, output schema goes first because it is easier to use the query afterwards, but when you write it, you first look at the input schema.

    This being said, the output first and input later looks odd to me in a "functional API", and since apparently this is true for the majority of people, if I can put my two cents, I would rather go with an inconsistent API (my understanding---I really tried to read everything but I failed---is that the bigger inconsistency is that output is sometimes not null?) than with something that people have problem understanding/using.

  25. TomFinley commented on Jan 23, 2019

    @TomFinley
    ContributorAuthor

    Would that make everything consistent?

    I think that falls apart for reasons of confusion, and maybe consistency, once we consider that some of these parameters are very often strings themselves. That is, your ... contains strings. (Which I know will happen lots of times, of course.)

    So, without checking, let me ask you a question about .NET. We will use a hypothetical simple example to guide our thinking.

    void Foo(string input, string optionalInput = null);
    void Foo(string input, string output, string optionalInput = null);

    Then imagine I have some other code further down that calls this method.

    Foo("hello", "friend");

    I'll tell you frankly, I don't know what will happen here. We might say I'm just ignorant, but I've been using .NET professionally for a decade now. I know a fair amount about its behavior, but this just seems confusing to me. What does the C# compiler do here? Does it complain? Does it favor the second one because that one is the one with two required arguments and I provided two arguments?

    If it prefers the second, let us imagine a user. They write this.

    Foo("hello");

    They read the documentation. They see the first overload. Then they say, "hey, this optional parameter does something I want, and it is optional. They naturally change it to be this.

    Foo("hello", "friend");

    But lurking in ths shadows is this overload you made me add.

    To be honest, I have absolutely no idea what C# does in the face of a situation like this. (I am about to find out now, but I wanted to write the above, to make the point that I, a 10 year C# veteran working with it day in day out, have absolutely no idea what will happen in this hypothetical API.

    So let me find out...

    public static class Program
    {
        public interface Bubba
        {
            void Foo(string input, string optionalInput = null);
            void Foo(string input, string output, string optionalInput = null);
        }
    
        private static void Main(string[] args)
        {
            Bubba bubs = default;
            bubs.Foo("hello", "friend");
        }
    }

    OK. Apparently against all my guesses, it did not complain, but it also did not prefer the second, it preferred the first. That I'd argue is even worse from this specific perspective. Let's imagine I am trying to map an input to an output column, it's not even clear to me at that point how I'd write that code. (I mean, I guess I could be explicit and write bubs.Foo("hello", "friend", null); or bubs.Foo("hello", output: "friend"); to be explicit.

    But, I'd argue that your proposal has, in this common case, led to a worse not a better world. So I'm afraid I not enthusiastic about this "overload" idea.

    (There's also the secondary point about it entailing a lot of work, etc. etc. etc., which we ought not to be blind to, but that's a secondary point.)

    This being said, the output first and input later looks odd to me in a "functional API", and since apparently this is true for the majority of people, if I can put my two cents, I would rather go with an inconsistent API (my understanding---I really tried to read everything but I failed---is that the bigger inconsistency is that output is sometimes not null?) than with something that people have problem understanding/using.

    I have a different view. Adjustment of initial prejudices to "get" a consistent API, even if this hypothetical person initially has trouble "getting" it, is a cost paid once. Being inconsistent is a cost paid forever. I have a clear preference in this case.

    But, if all the .NET people are screaming at me that I'm wrong, they know API design better than I do, so... whatevs. Do what you want.

  26. sfilipi commented on Jan 23, 2019

    @sfilipi
    Member

    After discussing this with @Ivanidzo4ka @TomFinley @eerhardt @markusweimer the decision is to:

    • change the current extension APIs and transformer estimator constructors to have the generated column first, and the input columns come after all required parameters.
    • the name of the column resulting from the transformation will be "outputColumnName" and the name of the column/columns to apply the transformation too would be "inputColumnName".
    • the "inputColumnName" parameter will default to null, for transformations that can operate on a single column. For this case, the "outputColumnName" shall be set by the user to be the name of the column of the dataset that they want to transform [sigh..] for the pipeline to work.

    @stephentoub @Zruty0 @glebuk @zeahmed @justinormont @KrzysztofCwalina @interesaaat thank you all for your input.

    cc @terrajobst in case he wants to discuss it further on the API review meetings.
    cc @CESARDELATORRE

  27. sfilipi commented on Jan 30, 2019

    @sfilipi
    Member

    Fixed with #2239

  28. added this to the 0119 milestone on Jan 31, 2019
  29. ghost locked as resolved and limited conversation to collaborators on Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions