-
Notifications
You must be signed in to change notification settings - Fork 1.9k
PipelineSweeperMacro for Multi-Class Classification #539
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 2 commits
41f8598
b7653de
0ccfbab
46af33f
d7e8402
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 |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Reflection; | ||
| using Microsoft.ML.Runtime.CommandLine; | ||
| using Microsoft.ML.Runtime.EntryPoints; | ||
| using Microsoft.ML.Runtime.Data; | ||
| using Microsoft.ML.Runtime.PipelineInference; | ||
| using Microsoft.ML.Runtime.EntryPoints.JsonUtils; | ||
| using Newtonsoft.Json.Linq; | ||
|
|
||
| namespace Microsoft.ML.Runtime.PipelineInference | ||
| { | ||
| /// <summary> | ||
| /// PipelineSweeper will support metrics as they are added here. | ||
| /// </summary> | ||
| public sealed class PipelineSweeperSupportedMetrics | ||
| { | ||
| public enum Metrics | ||
| { | ||
| Auc, | ||
| AccuracyMicro, | ||
| AccuracyMacro, | ||
| L1, | ||
| L2, | ||
| F1, | ||
| AuPrc, | ||
| TopKAccuracy, | ||
| Rms, | ||
| LossFn, | ||
| RSquared, | ||
| LogLoss, | ||
| LogLossReduction, | ||
| Ndcg, | ||
| Dcg, | ||
| PositivePrecision, | ||
| PositiveRecall, | ||
| NegativePrecision, | ||
| NegativeRecall, | ||
| DrAtK, | ||
| DrAtPFpr, | ||
| DrAtNumPos, | ||
| NumAnomalies, | ||
| ThreshAtK, | ||
| ThreshAtP, | ||
| ThreshAtNumPos, | ||
| Nmi, | ||
| AvgMinScore, | ||
| Dbi | ||
| }; | ||
|
|
||
| /// <summary> | ||
| /// Mapp Enum Metrics to a SupportedMetric | ||
| /// </summary> | ||
| private static readonly Dictionary<string, SupportedMetric> _map = new Dictionary<string, SupportedMetric> | ||
|
Member
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.
if the only usage for this is to contain a list of known metrics, the enum might be enough.
Member
Author
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. To specify the metric argument in the Macro we are using an Enum. That's the list of metrics you see in he CSharpApi.cs file. However, for parsing the results from the train test macro we need to use the metric names used by the ML.NET evaluators. e.g. AcuracyMicro (Enum Argument) v/s 'Accuracy(micro-avg)' (Evaluator metric). This dictionary maps the two. Does that clarify why we need this mapping? In reply to: 203217600 [](ancestors = 203217600) 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 think Senja meant that the conversion from the enum to a string is not needed (see my comment below on the GetSupportedMetric method). In reply to: 203220227 [](ancestors = 203220227,203217600) |
||
| { | ||
| { Metrics.Auc.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Auc, true)}, | ||
| { Metrics.AccuracyMicro.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.AccuracyMicro, true)}, | ||
| { Metrics.AccuracyMacro.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.AccuracyMacro, true)}, | ||
| { Metrics.L1.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.L1, false)}, | ||
| { Metrics.L2.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.L2, false)}, | ||
| { Metrics.F1.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.F1, true)}, | ||
| { Metrics.AuPrc.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.AuPrc, true)}, | ||
| { Metrics.TopKAccuracy.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.TopKAccuracy, true)}, | ||
| { Metrics.Rms.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Rms, false)}, | ||
| { Metrics.LossFn.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.LossFn, false)}, | ||
| { Metrics.RSquared.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.RSquared, false)}, | ||
| { Metrics.LogLoss.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.LogLoss, false)}, | ||
| { Metrics.LogLossReduction.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.LogLossReduction, true)}, | ||
| { Metrics.Ndcg.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Ndcg, true)}, | ||
| { Metrics.Dcg.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Dcg, true)}, | ||
| { Metrics.PositivePrecision.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.PositivePrecision, true)}, | ||
| { Metrics.PositiveRecall.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.PositiveRecall, true)}, | ||
| { Metrics.NegativePrecision.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.NegativePrecision, true)}, | ||
| { Metrics.NegativeRecall.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.NegativeRecall, true)}, | ||
| { Metrics.DrAtK.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.DrAtK, true)}, | ||
| { Metrics.DrAtPFpr.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.DrAtPFpr, true)}, | ||
| { Metrics.DrAtNumPos.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.DrAtNumPos, true)}, | ||
| { Metrics.NumAnomalies.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.NumAnomalies, true)}, | ||
| { Metrics.ThreshAtK.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.ThreshAtK, false)}, | ||
| { Metrics.ThreshAtP.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.ThreshAtP, false)}, | ||
| { Metrics.ThreshAtNumPos.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.ThreshAtNumPos, false)}, | ||
| { Metrics.Nmi.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Nmi, false)}, | ||
| { Metrics.AvgMinScore.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.AvgMinScore, false)}, | ||
| { Metrics.Dbi.ToString(), new SupportedMetric(FieldNames.PipelineSweeperSupportedMetrics.Dbi, false)} | ||
| }; | ||
|
|
||
| public static SupportedMetric GetSupportedMetric(IHostEnvironment env, string metricName) | ||
|
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.
Is this argument needed? If it's just used for the one check below, we can either have an IExceptionContext instead, or simply use Contracts.CheckNonEmpty(). #Resolved
Member
Author
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 removed the argument. Is there a suitable Contracts.Check* API that is used for an Enum argument? For now, I just removed the Checks since the default for the switch/case will throw a NotSupportedException anyway In reply to: 203461762 [](ancestors = 203461762) 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.
You can pass a Metric here instead of a string, this way you won't need the dictionary, and instead just do a switch statement on the parameter. #Resolved
Member
Author
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. Thanks for the clarification. Modified to using switch case as suggested. In general, regarding usage of switch-case v.s. a dictionary mapping - is there some concrete benefit of using switch-case here or is it mostly a matter of personal preferance ? In reply to: 203462492 [](ancestors = 203462492)
Member
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. Maybe to avoid the dictionary & switch statement, you can just keep a whitelist of the maximizing metrics. In the GetSupportedMetric, you can retrieve the boolean from looking up the whitelist. In reply to: 203543398 [](ancestors = 203543398,203462492)
Member
Author
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. Hi Senja. I intend to re-factor this code (task 267452). I want to unblock the benchmarking efforts . Is it fine if i keep it as switch-case for now? In reply to: 203780367 [](ancestors = 203780367,203543398,203462492) |
||
| { | ||
| Contracts.CheckValue(env, nameof(env)); | ||
| env.CheckNonEmpty(metricName, nameof(metricName)); | ||
|
|
||
| if (_map.ContainsKey(metricName)) | ||
| { | ||
| return _map[metricName]; | ||
| } | ||
|
|
||
| throw new NotSupportedException($"Metric '{metricName}' not supported."); | ||
| } | ||
| } | ||
|
|
||
| public sealed class SupportedMetric | ||
| { | ||
| public string Name { get; } | ||
| public bool IsMaximizing { get; } | ||
|
|
||
| public SupportedMetric(string name, bool isMaximizing) | ||
| { | ||
| Name = name; | ||
| IsMaximizing = isMaximizing; | ||
| } | ||
|
|
||
| public override string ToString() => Name; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15626,36 +15626,37 @@ public sealed class UniformRandomAutoMlEngine : AutoMlEngine | |
|
|
||
| public abstract class AutoMlStateBase : ComponentKind {} | ||
|
|
||
| public enum AutoInferenceAutoMlMlStateArgumentsMetrics | ||
| public enum PipelineSweeperSupportedMetricsMetrics | ||
|
Contributor
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. Isn't MetricsMetrics a bit odd sounding? #Resolved
Member
Author
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. This is the auto generated file (the CSharpAPI translation layer) . The name here results from concatenating the following from PipelineSweeperSupportedMetrics.cs public sealed class PipelineSweeperSupportedMetrics { public enum Metrics {The name used for the class / enum look fine to me. Any alternate suggestions ? In reply to: 203265071 [](ancestors = 203265071) |
||
| { | ||
| Auc = 0, | ||
| AccuracyMicro = 1, | ||
| AccuracyMacro = 2, | ||
| L2 = 3, | ||
| F1 = 4, | ||
| AuPrc = 5, | ||
| TopKAccuracy = 6, | ||
| Rms = 7, | ||
| LossFn = 8, | ||
| RSquared = 9, | ||
| LogLoss = 10, | ||
| LogLossReduction = 11, | ||
| Ndcg = 12, | ||
| Dcg = 13, | ||
| PositivePrecision = 14, | ||
| PositiveRecall = 15, | ||
| NegativePrecision = 16, | ||
| NegativeRecall = 17, | ||
| DrAtK = 18, | ||
| DrAtPFpr = 19, | ||
| DrAtNumPos = 20, | ||
| NumAnomalies = 21, | ||
| ThreshAtK = 22, | ||
| ThreshAtP = 23, | ||
| ThreshAtNumPos = 24, | ||
| Nmi = 25, | ||
| AvgMinScore = 26, | ||
| Dbi = 27 | ||
| L1 = 3, | ||
| L2 = 4, | ||
| F1 = 5, | ||
| AuPrc = 6, | ||
| TopKAccuracy = 7, | ||
| Rms = 8, | ||
| LossFn = 9, | ||
| RSquared = 10, | ||
| LogLoss = 11, | ||
| LogLossReduction = 12, | ||
| Ndcg = 13, | ||
| Dcg = 14, | ||
| PositivePrecision = 15, | ||
| PositiveRecall = 16, | ||
| NegativePrecision = 17, | ||
| NegativeRecall = 18, | ||
| DrAtK = 19, | ||
| DrAtPFpr = 20, | ||
| DrAtNumPos = 21, | ||
| NumAnomalies = 22, | ||
| ThreshAtK = 23, | ||
| ThreshAtP = 24, | ||
| ThreshAtNumPos = 25, | ||
| Nmi = 26, | ||
| AvgMinScore = 27, | ||
| Dbi = 28 | ||
|
Member
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.
the numbers are implied, if they are just sequential. #Resolved
Member
Author
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. Looks like they are auto generated by the CSharp API generator. We did not specify the numbers in the Enum. In reply to: 203217022 [](ancestors = 203217022)
Contributor
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. If we serialize the enum, I think we'd want a fixed number. This could be serialized as a state object for the sweep so that the user can resume the sweep w/ the history of the sweep stored on disk. #Resolved
Member
Author
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. Not sure if this is a general comment , or something that needs to be addressed ? Btw, this is is auto generated file . The Enum you see here is defined in PipelineSweeperSupportedMetrics.cs In reply to: 203267499 [](ancestors = 203267499)
Contributor
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. It's a topic to consider for the future. |
||
| } | ||
|
|
||
|
|
||
|
|
@@ -15668,7 +15669,7 @@ public sealed class AutoMlStateAutoMlStateBase : AutoMlStateBase | |
| /// <summary> | ||
| /// Supported metric for evaluator. | ||
| /// </summary> | ||
| public AutoInferenceAutoMlMlStateArgumentsMetrics Metric { get; set; } = AutoInferenceAutoMlMlStateArgumentsMetrics.Auc; | ||
| public PipelineSweeperSupportedMetricsMetrics Metric { get; set; } = PipelineSweeperSupportedMetricsMetrics.Auc; | ||
|
|
||
| /// <summary> | ||
| /// AutoML engine (pipeline optimizer) that generates next candidates. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21644,6 +21644,7 @@ | |
| "Auc", | ||
| "AccuracyMicro", | ||
| "AccuracyMacro", | ||
| "L1", | ||
| "L2", | ||
| "F1", | ||
| "AuPrc", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
If you change the GetSupportedMetric method to take the Metrics value, then you won't need this, you can just pass args.Metric. #Resolved