Metadata utils internalization, migration of few useful methods #2651
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…c facing metadata methods.
…members to public.
TomFinley
requested review from
codemzs,
Ivanidzo4ka,
eerhardt,
yaeldekel,
artidoro and
sfilipi
Codecov Report
@@ Coverage Diff @@
## master #2651 +/- ##
==========================================
- Coverage 71.54% 71.54% -0.01%
==========================================
Files 800 801 +1
Lines 141848 141850 +2
Branches 16119 16120 +1
==========================================
- Hits 101482 101481 -1
- Misses 35916 35917 +1
- Partials 4450 4452 +2
|
yaeldekel
reviewed
Feb 20, 2019
| @@ -998,16 +998,17 @@ private protected override IEnumerable<string> GetPerInstanceColumnsToSave(RoleM | |||
| // Multi-class evaluator adds four per-instance columns: "Assigned", "Top scores", "Top classes" and "Log-loss". | |||
| private protected override IDataView GetPerInstanceMetricsCore(IDataView perInst, RoleMappedSchema schema) | |||
| { | |||
| // If the label column is a key without key values, convert it to I8, just for saving the per-instance | |||
| // If the label column is a key without text key values, convert it to I8, just for saving the per-instance | |||
There was a problem hiding this comment.
I noticed that the code actually converts to R8, not I8.
There was a problem hiding this comment.
Ahhh. Delightful... seems a pity to update such a simple PR for an internal class. How about this, I'll locally fix it, and if for some other reason I need to update the PR it'll go in, otherwise it'll just go into my next PR.
eerhardt
reviewed
Feb 20, 2019
| /// </summary> | ||
| public static bool IsNormalized(this DataViewSchema.Column column) | ||
| { | ||
| var metaColumn = column.Metadata.Schema.GetColumnOrNull((MetadataUtils.Kinds.IsNormalized)); |
There was a problem hiding this comment.
(super nit): double params here. Can remove one set.
eerhardt
reviewed
Feb 20, 2019
|
|
||
| [BestFriend] | ||
| internal static bool HasKeyValues(this SchemaShape.Column col) | ||
| public static bool HasKeyValues(this SchemaShape.Column col) |
There was a problem hiding this comment.
Just for my information - what's the difference between this method and the public static bool HasKeyValues in SchemaAnnotationsExtensions?
I can see they contain different code, but they are named the same...?
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2622 by limiting the public surface area of metadata/annotations conveniences to a few places.
Note that that migrated methods are not exactly the same as the existing methods in the case of querying whether a key column type exists, since the existing methods predated the schema column type and therefore had some unnecessary parameters. Though I decided to expand it a bit so as to allow checks on whether other types of key values are present. Not sure if that's necessary or helpful.
Also internalized the rather innocuous but nonetheless probably unnecessary metadata builder extensions.
Commits structured incrementally as usual for the small steps.