Skip to content

Added convenience constructor for set of transforms (#380). #405

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 2, 2018
Merged

Added convenience constructor for set of transforms (#380). #405

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bfac765
Added convenience constructor for set of transforms (#371).
zeahmed Jun 25, 2018
20d9194
Removed useless validation from Concate transform.
zeahmed Jun 25, 2018
44e8e8d
Added more parameters to some transforms.
zeahmed Jun 25, 2018
d3c627e
Addressed reviewers' comments.
zeahmed Jun 26, 2018
5a34a16
XML Comments added to constructors/helper methods.
zeahmed Jun 26, 2018
1566ed0
Created private static class for managing default values.
zeahmed Jun 26, 2018
e9b1023
Addressed reviewers' comments.
zeahmed Jun 26, 2018
05637d0
Resolved some formatting issues.
zeahmed Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Microsoft.ML.Data/Transforms/ConcatTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,28 @@ private static VersionInfo GetVersionInfo()

public override ISchema Schema => _bindings;

public ConcatTransform(IHostEnvironment env, IDataView input, string outputColumn, params string[] inputColumns)
Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string outputColumn, params string[] inputColumns [](start = 70, length = 49)

This usage is inconsistent with your usage elsewhere, where the source is first, and the names of the output is second.

I would recommend actually making name of output first, and name of inputs second (as you have done here)... since I think the situation where you have multiple params to indicate input columns is more common. Indeed I cannot think of a single case where a params describes output columns, but I can think of instances where it describes input columns. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed to "name" and "source" as well to be consistent with their naming in Argument object.

In reply to: 197985474 [](ancestors = 197985474)

: base(env, RegistrationName, input)
{
var cols = new Column[1];
cols[0] = new Column()
{
Name = outputColumn,
Source = inputColumns
};

var args = new Arguments()
{
Column = cols
};
Host.CheckValue(args, nameof(args));
Host.CheckUserArg(Utils.Size(args.Column) > 0, nameof(args.Column));
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jun 25, 2018 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why we do it other constructor, because we get them as parameters, and you need to validate them, but here, you create args and it content specifically.
I would rather have validation of outputColumn, inputColumns, rather than copypaste validation from other constructor. (input get checked in base constructor) #Resolved

Copy link
Contributor Author

@zeahmed zeahmed Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense! #Resolved

for (int i = 0; i < args.Column.Length; i++)
Host.CheckUserArg(Utils.Size(args.Column[i].Source) > 0, nameof(args.Column));

_bindings = new Bindings(args.Column, null, Source.Schema);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jun 25, 2018 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

args.Column [](start = 37, length = 11)

what's the point to create args, if you only use it property? #Resolved

}

/// <summary>
/// Public constructor corresponding to SignatureDataTransform.
/// </summary>
Expand Down
26 changes: 26 additions & 0 deletions src/Microsoft.ML.Data/Transforms/CopyColumnsTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,32 @@ private static VersionInfo GetVersionInfo()

private const string RegistrationName = "CopyColumns";

public static CopyColumnsTransform Create(IHostEnvironment env, IDataView input, params string[] inputColumns)
{
var inputOutputColumns = new(string inputColumn, string outputColumn)[inputColumns.Length];
for (int i = 0; i < inputColumns.Length; i++)
{
inputOutputColumns[i].inputColumn = inputOutputColumns[i].outputColumn = inputColumns[i];
}
return Create(env, input, inputOutputColumns);
Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only practical use of the copy transform is to, effectively, rename columns. Therefore, handling the case where name of the output is identical to that of the input seems to serve no purpose. #Closed

}

public static CopyColumnsTransform Create(IHostEnvironment env, IDataView input, params (string inputColumn, string outputColumn)[] inputOutputColumns)
{
Column[] cols = new Column[inputOutputColumns.Length];
for (int i = 0; i < inputOutputColumns.Length; i++)
{
cols[i] = new Column();
cols[i].Source = inputOutputColumns[i].inputColumn;
cols[i].Name = inputOutputColumns[i].outputColumn;
}
var args = new Arguments()
{
Column = cols
};
return new CopyColumnsTransform(env,args,input);
}
Copy link
Contributor

@TomFinley TomFinley Jun 27, 2018 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank lines with trailing whitespace on them is evil. #Resolved


public CopyColumnsTransform(IHostEnvironment env, Arguments args, IDataView input)
: base(env, RegistrationName, env.CheckRef(args, nameof(args)).Column, input, null)
{
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.ML.Data/Transforms/DropColumnsTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,16 @@ private static VersionInfo GetVersionInfo()
private const string DropRegistrationName = "DropColumns";
private const string KeepRegistrationName = "KeepColumns";

public DropColumnsTransform CreateColumnDroper(IHostEnvironment env, IDataView input, params string[] columnsToDrop)
{
return new DropColumnsTransform(env, new Arguments() { Column = columnsToDrop }, input);
}

public static DropColumnsTransform CreateColumnSelector(IHostEnvironment env, IDataView input, params string[] columnsToKeep)
{
return new DropColumnsTransform(env, new KeepArguments() { Column = columnsToKeep }, input);
}
Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep columns, we do DropColumnsTransform.CreateColumnSelector. :) The way that we resolve this on the command line, entry-points, the GUI, and whatnot, is we have this "KeepColumnsTransform" loadname declared, to make it look like there is another transform.

The fact that we do so in the underlying C# code is just because previously this little bit of confusion didn't matter, since we weren't advocating that people use this directly. However since we now are, we ought to adjust accordingly.

For that reason, I wonder if we can make the relationship explicit...

public static class KeepColumnsTransform
{
    public IDataTransform Create(IHostEnvironment env, IDataView input, params string[] columnsToKeep)
        => new DropColumnsTransform(env, new KeepArguments() { Column = columnsToKeep }, input);
}

or something along these lines.

Once that ambiguity is resolved, we can change "drop" creator back to a plain old constructor. #Closed


/// <summary>
/// Public constructor corresponding to SignatureDataTransform.
/// </summary>
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.ML.Data/Transforms/NAFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ private static VersionInfo GetVersionInfo()
private readonly bool _complement;
private const string RegistrationName = "MissingValueFilter";

public NAFilter(IHostEnvironment env, IDataView input, params string[] inputColumns)
: this(env, new Arguments() { Column = inputColumns}, input)
{

}

public NAFilter(IHostEnvironment env, Arguments args, IDataView input)
: base(env, RegistrationName, input)
{
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.ML.Transforms/BootstrapSampleTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ public BootstrapSampleTransform(IHostEnvironment env, Arguments args, IDataView
_poolSize = args.PoolSize;
}

public BootstrapSampleTransform(IHostEnvironment env, IDataView input, bool complement = false, uint? seed = null, bool shuffleInput = true, int poolSize = 1000)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Jun 25, 2018 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool complement = false, uint? seed = null, bool shuffleInput = true, int poolSize = 1000 [](start = 79, length = 89)

out of curiosity, why argument object is bad solution? why we want to flatten that object in set of parameters?
I'm also concern what we will have to support this defaults in two places, and it really error prone. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that transform can be neatly instantiated on one line. Otherwise for every transform a relevant "Argument" object is needed to be created first which is also another option.

In reply to: 197909492 [](ancestors = 197909492)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see why it can be helpful in case if you have only Column object inside argument class, you add one more constructor which is nice to use. But in this particular case, i don't see any advantages of using this constructor, and it makes code maintainability much harder.
Also compare this constructor with Cathash constructor.
In cathash you hide all other parameters from argument class other than columns, and here you expose them.
Can we be consistent?

I don't mind to have constructor which will take only Columns information and will use default parameters for internal argument class, but make sure to proper comment which indicates what if user needs more, it need to use Arguments class

In reply to: 197912065 [](ancestors = 197912065,197909492)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but again, feel free to bring heavy artillery (@TomFinley)

In reply to: 197921908 [](ancestors = 197921908,197912065,197909492)

Copy link
Contributor Author

@zeahmed zeahmed Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case, definitely constructor can be removed. But I have a concern regarding if user has to instantiate an Argument object for a very simple case e.g. for all default values.

Another option could be

public BootstrapSampleTransform(IHostEnvironment env, IDataView input)
: this(env, new Arguments() { }, input)
{
}

Yes, for CatHash more parameters can be added. Waiting for more comments on this. #Closed

Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited by zeahmed
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I actually rather liked the constructor as it stands -- I do not consider the proposed replacement BootstrapSampleTransform(IHostEnvironment env, IDataView input) acceptable because the bootstrap sampler I expect is one of those things that, when it is used, will be used in a pair: first to get the in-bag sample (for training), then to get the out of bag sample (for validation and testing). Whether we expose the other things as parameters or not, I am less certain about the utility of these.

@Ivanidzo4ka , is the concern with having a convenience constructor with default arguments, that the defaults could diverge? If that is the case, would perhaps prefer constants (either directly as private const in the transform class itself, or perhaps more ideally as public const in a nested private static class named Defaults or something).

If the point is rather that there should be exactly one way to do this, I'm not entirely certain I agree -- the arguments object may need to exist for the command line parser and entry-points which (being reflection driven) require total uniformity in how things are instantiated, but this bears very little resemblance to how someone, if trying to write a .NET API, would actually write code. If it is a small "shim" as it is now over the arguments, I'm not sure I see the harm, and I see a considerable benefit. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't see much reason to have constructor which duplicate other constructor with one difference: instead of one object we accept all the fields of this object.
I just don't understand why it's better and what convenience it brings.
I'm perfectly fine with something like

class(main_option)
class(main_option, secondary_parameter)
i just don't understand why you want to have
class(A,B,C,D,E,F)
and
class(argument_object) where argument_object has fields A,B,C,D,E,F

In reply to: 197983331 [](ancestors = 197983331)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Where they can be applied, the new idioms are clearly easier to use and understand.

In reply to: 198273557 [](ancestors = 198273557,197983331)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is based on the convenience you get in calling constructor. In my point of view, calling

new BootstrapSampleTransform(env, input, Complement,  Seed, ShuffleInput, PoolSize);

is more convenient and elegant than calling

new BootstrapSampleTransform(env, input, new Arguments() { Complement = complement, Seed = seed, ShuffleInput = shuffleInput, PoolSize = poolSize });

In reply to: 198273557 [](ancestors = 198273557,197983331)

: this(env, new Arguments() { Complement = complement, Seed = seed, ShuffleInput = shuffleInput, PoolSize = poolSize }, input)
{

}

private BootstrapSampleTransform(IHost host, ModelLoadContext ctx, IDataView input)
: base(host, input)
{
Expand Down
26 changes: 26 additions & 0 deletions src/Microsoft.ML.Transforms/CategoricalHashTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,32 @@ public sealed class Arguments : TransformInputBase

public const string UserName = "Categorical Hash Transform";

public static IDataTransform Create(IHostEnvironment env, IDataView input, params string[] inputColumns)
{
var inputOutputColumns = new(string inputColumn, string outputColumn)[inputColumns.Length];
for (int i = 0; i < inputColumns.Length; i++)
{
inputOutputColumns[i].inputColumn = inputOutputColumns[i].outputColumn = inputColumns[i];
}
return Create(env, input, inputOutputColumns);
}

public static IDataTransform Create(IHostEnvironment env, IDataView input, params (string inputColumn, string outputColumn)[] inputOutputColumns)
{
Column[] cols = new Column[inputOutputColumns.Length];
for (int i = 0; i < inputOutputColumns.Length; i++)
{
cols[i] = new Column();
cols[i].Source = inputOutputColumns[i].inputColumn;
cols[i].Name = inputOutputColumns[i].outputColumn;
}
var args = new Arguments()
{
Column = cols
};
return Create(env, args, input);
}

public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)
{
Contracts.CheckValue(env, nameof(env));
Expand Down
26 changes: 26 additions & 0 deletions src/Microsoft.ML.Transforms/CategoricalTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,32 @@ public Arguments()

public const string UserName = "Categorical Transform";

public static IDataTransform Create(IHostEnvironment env, IDataView input, params string[] inputColumns)
{
var inputOutputColumns = new (string inputColumn, string outputColumn)[inputColumns.Length];
for (int i = 0; i < inputColumns.Length; i++)
{
inputOutputColumns[i].inputColumn = inputOutputColumns[i].outputColumn = inputColumns[i];
}
return Create(env, input, inputOutputColumns);
}

public static IDataTransform Create(IHostEnvironment env, IDataView input, params (string inputColumn, string outputColumn)[] inputOutputColumns)
{
Column[] cols = new Column[inputOutputColumns.Length];
for (int i = 0; i < inputOutputColumns.Length; i++)
{
cols[i] = new Column();
cols[i].Source = inputOutputColumns[i].inputColumn;
cols[i].Name = inputOutputColumns[i].outputColumn;
}
var args = new Arguments()
{
Column = cols
};
return Create(env, args, input);
}

public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)
{
Contracts.CheckValue(env, nameof(env));
Expand Down
10 changes: 10 additions & 0 deletions src/Microsoft.ML.Transforms/CountFeatureSelection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ public sealed class Arguments : TransformInputBase

internal static string RegistrationName = "CountFeatureSelectionTransform";

public static IDataTransform Create(IHostEnvironment env, IDataView input, long count = 1, params string[] columns)
{
var args = new Arguments()
{
Column = columns,
Count = count
};
return Create(env, args, input);
}

/// <summary>
/// Create method corresponding to SignatureDataTransform.
/// </summary>
Expand Down
66 changes: 64 additions & 2 deletions src/Microsoft.ML.Transforms/GcnTransform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,37 @@ private static VersionInfo GetVersionInfo()

private readonly ColInfoEx[] _exes;

public static IDataTransform CreateGlobalContrastNormalizer(IHostEnvironment env, IDataView input, params string[] inputColumns)
{
var inputOutputColumns = new(string inputColumn, string outputColumn)[inputColumns.Length];
for (int i = 0; i < inputColumns.Length; i++)
{
inputOutputColumns[i].inputColumn = inputOutputColumns[i].outputColumn = inputColumns[i];
}
return CreateGlobalContrastNormalizer(env, input, inputOutputColumns);
}

public static IDataTransform CreateGlobalContrastNormalizer(IHostEnvironment env, IDataView input, params (string inputColumn, string outputColumn)[] inputOutputColumns)
{
GcnColumn[] cols = new GcnColumn[inputOutputColumns.Length];
for (int i = 0; i < inputOutputColumns.Length; i++)
{
cols[i] = new GcnColumn();
cols[i].Source = inputOutputColumns[i].inputColumn;
cols[i].Name = inputOutputColumns[i].outputColumn;
}
var args = new GcnArguments()
{
Column = cols
};
return new LpNormNormalizerTransform(env, args, input);
}

public static IDataTransform CreateGlobalContrastNormalizer(IHostEnvironment env, IDataView input, GcnArguments args)
{
return new LpNormNormalizerTransform(env, args, input);
}
Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain I see the utility in this specific constructor. #Closed


/// <summary>
/// Public constructor corresponding to SignatureDataTransform.
/// </summary>
Expand All @@ -263,9 +294,40 @@ public LpNormNormalizerTransform(IHostEnvironment env, GcnArguments args, IDataV
SetMetadata();
}

public static IDataTransform CreateLpNormNormalizer(IHostEnvironment env, IDataView input, params string[] inputColumns)
{
var inputOutputColumns = new(string inputColumn, string outputColumn)[inputColumns.Length];
for (int i = 0; i < inputColumns.Length; i++)
{
inputOutputColumns[i].inputColumn = inputOutputColumns[i].outputColumn = inputColumns[i];
}
return CreateLpNormNormalizer(env, input, inputOutputColumns);
}

public static IDataTransform CreateLpNormNormalizer(IHostEnvironment env, IDataView input, params (string inputColumn, string outputColumn)[] inputOutputColumns)
{
Column[] cols = new Column[inputOutputColumns.Length];
for (int i = 0; i < inputOutputColumns.Length; i++)
{
cols[i] = new Column();
cols[i].Source = inputOutputColumns[i].inputColumn;
cols[i].Name = inputOutputColumns[i].outputColumn;
}
var args = new Arguments()
{
Column = cols
};
return new LpNormNormalizerTransform(env, args, input);
}

public static IDataTransform CreateLpNormNormalizer(IHostEnvironment env, IDataView input, Arguments args)
{
return new LpNormNormalizerTransform(env, args, input);
}
Copy link
Contributor

@TomFinley TomFinley Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems to be of limited utility. #Closed


public LpNormNormalizerTransform(IHostEnvironment env, Arguments args, IDataView input)
: base(env, RegistrationName, env.CheckRef(args, nameof(args)).Column,
input, TestIsFloatVector)
: base(env, RegistrationName, env.CheckRef(args, nameof(args)).Column,
input, TestIsFloatVector)
{
Host.AssertNonEmpty(Infos);
Host.Assert(Infos.Length == Utils.Size(args.Column));
Expand Down
Loading