-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added convenience constructors for set of transforms. #491
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
Changes from 1 commit
16f7883
9c8ca3d
027ed32
3f7713d
1414750
1580c20
6dc2bc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -108,6 +108,16 @@ public bool TryUnparse(StringBuilder sb) | |||
|
|
||||
| public class Arguments : TransformInputBase | ||||
| { | ||||
| public Arguments() | ||||
| { | ||||
|
|
||||
| } | ||||
|
|
||||
| public Arguments(string name, string source) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm. What's this constructor for? #Closed |
||||
| { | ||||
| Column = new[] { new Column() { Source = source ?? name, Name = name } }; | ||||
| } | ||||
|
|
||||
| [Argument(ArgumentType.Multiple | ArgumentType.Required, HelpText = "New column definition(s) (optional form: name:type:src)", ShortName = "col", SortOrder = 1)] | ||||
| public Column[] Column; | ||||
|
|
||||
|
|
@@ -169,6 +179,25 @@ private static VersionInfo GetVersionInfo() | |||
| // This is parallel to Infos. | ||||
| private readonly ColInfoEx[] _exes; | ||||
|
|
||||
| /// <summary> | ||||
| /// Convenience constructor for public facing API. | ||||
| /// </summary> | ||||
| /// <param name="env">Host Environment.</param> | ||||
| /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param> | ||||
| /// <param name="name">Name of the output column.</param> | ||||
| /// <param name="source">Name of the column to be converted. If this is null '<paramref name="name"/>' will be used.</param> | ||||
| /// <param name="resultType">The expected type of the converted column.</param> | ||||
| /// <param name="keyRange">For a key column, this defines the range of values.</param> | ||||
| public ConvertTransform(IHostEnvironment env, | ||||
| IDataView input, | ||||
| string name, | ||||
| string source = null, | ||||
| DataKind? resultType = null, | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hi @zeahmed thanks for doing this. Quick question, why is this nullable? What happens if it is null? #Closed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually this is interesting. Unlike some other transforms, this transform is one of those where we kind of do have a required argument. Hmmm. We need to think carefully about how we want to phrase the constructor here. We may need to refine my prior idea from #380 of having the signature be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe not. Maybe just putting them before
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding
So, I would suggest setting resultType = DataKind.Num or make it required.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make it required myself... I sort of view API usage as similar to programmatic usage, and if we view this "convert" as similar to a cast or something, the idea that there is a default type to cast to just seems silly. Even if maybe we want to allow it in command-line/GUI world. In reply to: 200468398 [](ancestors = 200468398) |
||||
| KeyRange keyRange = null) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think conversions among key ranges are common enough that they are worthy of a convenience constructor. #Closed |
||||
| : this(env, new Arguments(name, source) { ResultType = resultType, KeyRange = keyRange }, input) | ||||
| { | ||||
| } | ||||
|
|
||||
| public ConvertTransform(IHostEnvironment env, Arguments args, IDataView input) | ||||
| : base(env, RegistrationName, env.CheckRef(args, nameof(args)).Column, | ||||
| input, null) | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,6 +216,18 @@ public ColInfoEx(SlotDropper slotDropper, bool suppressed, ColumnType typeDst, i | |
|
|
||
| private readonly ColInfoEx[] _exes; | ||
|
|
||
| /// <summary> | ||
| /// Convenience constructor for public facing API. | ||
| /// </summary> | ||
| /// <param name="env">Host Environment.</param> | ||
| /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param> | ||
| /// <param name="name">Name of the output column.</param> | ||
| /// <param name="source">Name of the input column. If this is null '<paramref name="name"/>' will be used.</param> | ||
| public DropSlotsTransform(IHostEnvironment env, IDataView input, string name, string source = null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this is a transform that works well for conveniences. Certainly this specific constructor, which really does nothing, is not something we need right now. I'd rather not do drop slots for now. #Closed |
||
| : this(env, new Arguments() { Column = new[] { new Column() { Source = source ?? name, Name = name } } }, input) | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Public constructor corresponding to SignatureDataTransform. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,26 +33,48 @@ public sealed class HashTransform : OneToOneTransformBase, ITransformTemplate | |
| public const int NumBitsMin = 1; | ||
| public const int NumBitsLim = 32; | ||
|
|
||
| private static class Defaults | ||
| { | ||
| public const int HashBits = NumBitsLim - 1; | ||
| public const uint Seed = 314489979; | ||
| public const bool Ordered = false; | ||
| public const int InvertHash = 0; | ||
| } | ||
|
|
||
| public sealed class Arguments | ||
| { | ||
| public Arguments() | ||
| { | ||
|
|
||
| } | ||
|
|
||
| public Arguments(string name, string source) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see that in some circumstances, maybe having a specialized constructor on arguments could be useful, but why is it helpful here? #Closed |
||
| { | ||
| Column = new[] { new Column(){ | ||
| Source = source ?? name, | ||
| Name = name | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| [Argument(ArgumentType.Multiple, HelpText = "New column definition(s) (optional form: name:src)", ShortName = "col", | ||
| SortOrder = 1)] | ||
| public Column[] Column; | ||
|
|
||
| [Argument(ArgumentType.AtMostOnce, HelpText = "Number of bits to hash into. Must be between 1 and 31, inclusive", | ||
| ShortName = "bits", SortOrder = 2)] | ||
| public int HashBits = NumBitsLim - 1; | ||
| public int HashBits = Defaults.HashBits; | ||
|
|
||
| [Argument(ArgumentType.AtMostOnce, HelpText = "Hashing seed")] | ||
| public uint Seed = 314489979; | ||
| public uint Seed = Defaults.Seed; | ||
|
|
||
| [Argument(ArgumentType.AtMostOnce, HelpText = "Whether the position of each term should be included in the hash", | ||
| ShortName = "ord")] | ||
| public bool Ordered; | ||
| public bool Ordered = Defaults.Ordered; | ||
|
|
||
| [Argument(ArgumentType.AtMostOnce, HelpText = "Limit the number of keys used to generate the slot name to this many. 0 means no invert hashing, -1 means no limit.", | ||
| ShortName = "ih")] | ||
| public int InvertHash; | ||
| public int InvertHash = Defaults.InvertHash; | ||
| } | ||
|
|
||
| public sealed class Column : OneToOneColumn | ||
|
|
@@ -234,6 +256,25 @@ public override void Save(ModelSaveContext ctx) | |
| TextModelHelper.SaveAll(Host, ctx, Infos.Length, _keyValues); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Convenience constructor for public facing API. | ||
| /// </summary> | ||
| /// <param name="env">Host Environment.</param> | ||
| /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param> | ||
| /// <param name="name">Name of the output column.</param> | ||
| /// <param name="source">Name of the column to be transformed. If this is null '<paramref name="name"/>' will be used.</param> | ||
| /// <param name="hashBits">Number of bits to hash into. Must be between 1 and 31, inclusive.</param> | ||
| /// <param name="invertHash">Limit the number of keys used to generate the slot name to this many. 0 means no invert hashing, -1 means no limit.</param> | ||
| public HashTransform(IHostEnvironment env, | ||
| IDataView input, | ||
| string name, | ||
| string source = null, | ||
| int hashBits = Defaults.HashBits, | ||
| int invertHash = Defaults.InvertHash) | ||
| : this(env, new Arguments(name, source) { HashBits = hashBits, InvertHash = invertHash }, input) | ||
| { | ||
| } | ||
|
|
||
| public HashTransform(IHostEnvironment env, Arguments args, IDataView input) | ||
| : base(Contracts.CheckRef(env, nameof(env)), RegistrationName, env.CheckRef(args, nameof(args)).Column, | ||
| input, TestType) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,19 @@ private static VersionInfo GetVersionInfo() | |
| private readonly ColumnType[] _types; | ||
| private KeyToValueMap[] _kvMaps; | ||
|
|
||
| /// <summary> | ||
| /// Convenience constructor for public facing API. | ||
| /// </summary> | ||
| /// <param name="env">Host Environment.</param> | ||
| /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param> | ||
| /// <param name="name">Name of the output column.</param> | ||
| /// <param name="source">Name of the input column. If this is null '<paramref name="name"/>' will be used.</param> | ||
| public KeyToValueTransform(IHostEnvironment env, IDataView input, string name, string source = null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
half of your files formatted in this way, half is one parameter for each line, why? #Closed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the line is going to be long enough to fit into the view, I format every parameter on separate line otherwise not. For this particular, its fine to have it on one line. #Closed |
||
| : this(env, new Arguments() { Column = new[] { new Column() { Source = source ?? name, Name = name } } }, input) | ||
| { | ||
| } | ||
|
|
||
|
|
||
| /// <summary> | ||
| /// Public constructor corresponding to SignatureDataTransform. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,13 +64,18 @@ public bool TryUnparse(StringBuilder sb) | |
| } | ||
| } | ||
|
|
||
| private static class Defaults | ||
| { | ||
| public const int ClassIndex = 0; | ||
| } | ||
|
|
||
| public sealed class Arguments : TransformInputBase | ||
| { | ||
| [Argument(ArgumentType.Multiple, HelpText = "New column definition(s) (optional form: name:src)", ShortName = "col", SortOrder = 1)] | ||
| public Column[] Column; | ||
|
|
||
| [Argument(ArgumentType.AtMostOnce, HelpText = "Label of the positive class.", ShortName = "index")] | ||
| public int ClassIndex; | ||
| public int ClassIndex = Defaults.ClassIndex; | ||
| } | ||
|
|
||
| public static LabelIndicatorTransform Create(IHostEnvironment env, | ||
|
|
@@ -111,6 +116,23 @@ private static string TestIsMulticlassLabel(ColumnType type) | |
| return $"Label column type is not supported for binary remapping: {type}. Supported types: key, float, double."; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Convenience constructor for public facing API. | ||
| /// </summary> | ||
| /// <param name="env">Host Environment.</param> | ||
| /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param> | ||
| /// <param name="name">Name of the output column.</param> | ||
| /// <param name="source">Name of the input column. If this is null '<paramref name="name"/>' will be used.</param> | ||
| /// <param name="classIndex">Label of the positive class.</param> | ||
| public LabelIndicatorTransform(IHostEnvironment env, | ||
| IDataView input, | ||
| string name, | ||
| string source = null, | ||
| int classIndex = Defaults.ClassIndex) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that zero is being used as default value. Do you suggest making it required? #Resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. So, in these so-called I do not suggest changing arguments (though you could if you want and I think it would be a great improvement, I think it's really silly that it doesn't require usage to be explicit) but I would certainly require it in the helper constructor. There's nothing fundamentally privileged about the first value... #Closed |
||
| : this(env, new Arguments() { Column = new[] { new Column() { Source = source ?? name, Name = name } }, ClassIndex = classIndex }, input) | ||
| { | ||
| } | ||
|
|
||
| public LabelIndicatorTransform(IHostEnvironment env, Arguments args, IDataView input) | ||
| : base(env, LoadName, Contracts.CheckRef(args, nameof(args)).Column, | ||
| input, TestIsMulticlassLabel) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose column transform is one of those transforms that is pretty much completely useless unless it supports multiple columns. It's not like other transforms where you can just apply the transform again to get more columns. If something isn't in the choose list, it is simply dropped -- no second chances. We need to support multiple column specification in the constructor.
However, choose columns generally as far as I know could just do with a
params string[] nameor something, and be done with it. Generally it isn't used to rename columns. (That is, I seexf=choose{col=Foo,Bar,Biz}, I don't see muchxf=Choose{col=Foo:Bar}or something like this.) #ClosedUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually wait. What is the relationship between "choose columns transform" and "keep columns transform"? I think there is a subtle difference, but it escapes me at the moment. #Closed
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, renaming of chosen column is the difference between ChooseColumnsTransform and KeepColumnsTransform. KeepColumnsTransform does not support renaming and drops the rest of the columns. ChooseColumnsTransform does renaming as well as has other parameters like
Hiddenwhich has following options.What I suggest that for convenience this transform operate in similar way as KeepColumnsTransform.
For other usage, user can invoke constructor with
Argumentsobject. What do you suggest? #ClosedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably just keep the non-renaming case convenient, and nuke the constructor on arguments.
In reply to: 200471726 [](ancestors = 200471726)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that current changes are fine? If not can you please explain bit more?
In reply to: 200727095 [](ancestors = 200727095,200471726)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are good, thank you @zeahmed.
In reply to: 200743746 [](ancestors = 200743746,200727095,200471726)