-
Notifications
You must be signed in to change notification settings - Fork 1.9k
BinaryClassification Calibrators: Replace Preview API with IDataView based getter API. #3353
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
...samples/Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/Calibrators/FixedPlatt.cs
Outdated
Show resolved
Hide resolved
@@ -54,18 +53,30 @@ public static void Example() | |||
// Score 5.36571 Probability 0.9065308 | |||
} | |||
|
|||
private static void PrintRowViewValues(Microsoft.ML.Data.DataDebuggerPreview data) | |||
private static void PrintRowViewValues(IDataView transformedData) |
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.
There are four similar functions. Would it be better to use TT file? #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.
Agreed, TT file would be ideal not just for these functions but for all four samples. It should be a task of its own and I'll open one. This particular issue addresses preview API usage. The reason I want to open a different task is because next few days the focus will be on documentation and I don't know if it make sense to prioritize rewriting these four samples with TT files given they are not broken. I'll still try to create TT files if I find time between documentation work. Thanks, @wschin and let me know your thoughts. #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.
Spoke with @wschin offline and he is fine with my above proposal.
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.
Approved with some non-blocking comments.
Codecov Report
@@ Coverage Diff @@
## master #3353 +/- ##
==========================================
- Coverage 72.69% 72.69% -0.01%
==========================================
Files 807 807
Lines 145172 145172
Branches 16225 16225
==========================================
- Hits 105537 105533 -4
- Misses 35221 35224 +3
- Partials 4414 4415 +1
|
...samples/Microsoft.ML.Samples/Dynamic/Trainers/BinaryClassification/Calibrators/FixedPlatt.cs
Outdated
Show resolved
Hide 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.
towards #3350