-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Delete IColumn #1801
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
Delete IColumn #1801
Conversation
* Stop using IColumn in predicted label scorers. * Stop using IColumn in static schema shape analysis. * Stop using IColumn in linear model statistics creation.
* Stop using IColumn in FastTree statistics creation. * Stop using IColumn in benchmarking. * Stop using IColumn in many tests. * Add minor conveniences to metadata builder. * Allow metadata builder to have metadata of metadata. * Add appropriate validation in certain places. * Put warnings on an inappropriate method dealing with Batch that should not exist.
sfilipi
left a comment
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.
![]()
eerhardt
left a comment
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.
![]()
| /// <param name="type">The metadata type.</param> | ||
| /// <param name="getter">The getter delegate.</param> | ||
| public void Add<TValue>(string name, ColumnType type, ValueGetter<TValue> getter) | ||
| /// <param name="metadata">Metadata of the input column. Note that metadata on a metadata column is somewhat rare |
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.
What does input column mean here? Is it a metadata column (specified by name, type, getter) or a column which metadata will be attached to? Maybe change input column to metadata column added?
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.
Ah. I'll think about this and handle in next PR after this.
Fixes #1755.
At least initially staged as three commits, where we change some places, change the more difficult and rote places like tests and whatnot, then remove everything.