-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Internalize ITransformTemplate #2334
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 all commits
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 |
---|---|---|
|
@@ -63,10 +63,12 @@ public sealed override void Save(ModelSaveContext ctx) | |
[BestFriend] | ||
private protected abstract void SaveCore(ModelSaveContext ctx); | ||
|
||
/// <summary> | ||
/// For the ITransformTemplate implementation. | ||
/// </summary> | ||
public abstract IDataTransform ApplyToData(IHostEnvironment env, IDataView newSource); | ||
private protected abstract IDataTransform ApplyToDataCore(IHostEnvironment env, IDataView newSource); | ||
|
||
IDataTransform ITransformTemplate.ApplyToData(IHostEnvironment env, IDataView newSource) | ||
=> ApplyToDataCore(env, newSource); | ||
internal IDataView ApplyToData(IHostEnvironment env, IDataView newSource) | ||
=> ApplyToDataCore(env, newSource); | ||
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. Why do you need this second method? IDataTransform derives from IDataView. |
||
|
||
/// <summary> | ||
/// Derived classes provide the specific bindings object. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -682,7 +682,7 @@ public Row GetRow(Row input, Func<int, bool> active) | |
return new RowImpl(input, _mapper); | ||
} | ||
|
||
public IDataTransform ApplyToData(IHostEnvironment env, IDataView newSource) | ||
IDataTransform ITransformTemplate.ApplyToData(IHostEnvironment env, IDataView newSource) | ||
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 agree with you that this is probably the best policy since it even discourages internal code from using this now deprecated idiom, but just in case you are curious, it is not necessary to do this here since this nested class ( |
||
=> new SelectColumnsDataTransform(env, _transform, new Mapper(_transform, newSource.Schema), newSource); | ||
} | ||
|
||
|
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.
Until we can delete this altogether, it may be worthwhile to add a note here to explain that the preferred idioms is that things work via the new
ITransformer
mechanism, which is the successor of this sort of idiom. I might not suggest adding the[Obsolete]
tag -- that's a bit heaviweight -- just something in the XML docstring here like, "yeah, yeah, this is here, but we're trying to get rid of it, don't write new code with it."