-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove ISchematized interface from the codebase. #1759
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
Conversation
1. Remove ISchematized 2. For any class that requires a Schema, we add an Schema as its field 3. Rename Schema to OutputSchema in IRowToRowMapper
| loaderAssemblyName: typeof(RowToRowMapperTransform).Assembly.FullName); | ||
| } | ||
|
|
||
| public override Schema Schema => _bindings.Schema; |
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.
Schema Schema [](start = 24, length = 13)
We might want to hide this Schema behind an explicit interface implementation, e.g., IDataView.Schema or somesuch, since we don't want to emphasize the fact that this is an IDataView (it eventually will not be). The base class, whatever it is, could have a IDataView.Schema that references a [BestFriend] private protected abstract Schema DataSchema that is overridden here and elsewhere, if appropriate. Then people won't get these identical Schema and OutputSchema in their face, and it also clears the way to more easily deprecate the fact that this is also an IDataTransform in the future. #Resolved
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.
Thank you Tom! In TransformBase (where IDataTransform is inherited), we have
Schema IDataView.Schema => OutputSchema;
public abstract Schema OutputSchema { get; }for hidding IDataView.Schema. Do we still need [BestFriend] private protected abstract Schema DataSchema?
In reply to: 237585047 [](ancestors = 237585047)
src/Microsoft.ML.Data/Scorers/SchemaBindablePredictorWrapper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ML.StandardLearners/FactorizationMachine/FieldAwareFactorizationMachineUtils.cs
Show resolved
Hide resolved
|
You have changes in libmf submodule, is it necessary? #Resolved |
|
It's not! It's my turn making this mistake. Let me fix it. In reply to: 443316147 [](ancestors = 443316147) |
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.
Thanks for getting rid of this @wschin !
|
Fixing this problem is difficult because git automatically fetches the latest commit for the tracked branch. If you don't mind, let's just keep these changes. In reply to: 443316482 [](ancestors = 443316482,443316147) |
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.
![]()
Fixes #1502.