-
Notifications
You must be signed in to change notification settings - Fork 1.9k
add threshold for RCA #5218
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
add threshold for RCA #5218
Changes from 35 commits
2c7a62b
f9af073
0551ecd
3138c39
946ac43
d39e657
eadada4
6cf9b59
36d1625
0f81982
f3fad18
612be4d
23261f2
fa10bff
919ed6b
0407282
0efee95
bde4a53
a1ab905
711dfca
e202a04
1a2d569
b059b53
fa834fd
2b29426
0860e01
6cf15c8
a3eee1a
66261e6
887e992
421157d
54727ea
0e60d3c
9d4a45e
cd19889
28bf0a5
8e5784e
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 |
|---|---|---|
|
|
@@ -187,14 +187,15 @@ public static IDataView DetectEntireAnomalyBySrCnn(this AnomalyDetectionCatalog | |
| /// It is used when score is calculated for each root cause item. The range of beta should be in [0,1]. | ||
| /// For a larger beta, root cause items which have a large difference between value and expected value will get a high score. | ||
| /// For a small beta, root cause items which have a high relative change will get a low score.</param> | ||
| /// <param name="rootCauseThreshold">A threshold to determine whether the point should be root cause. If the point's delta is equal to or larger than rootCauseThreshold multiplied by anomaly dimension point's delta, this point is treated as a root cause. Different threshold will turn out different results. Users can choose the delta according to their data and requirments. </param> | ||
| /// <example> | ||
| /// <format type="text/markdown"> | ||
| /// <] | ||
| /// ]]> | ||
| /// </format> | ||
| /// </example> | ||
| public static RootCause LocalizeRootCause(this AnomalyDetectionCatalog catalog, RootCauseLocalizationInput src, double beta = 0.5) | ||
| public static RootCause LocalizeRootCause(this AnomalyDetectionCatalog catalog, RootCauseLocalizationInput src, double beta = 0.5, double rootCauseThreshold = 0.95) | ||
|
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. nit: another option as suggested by Harish (for AD) is to create an Options class to avoid multiple overload in future. #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 found that the option is always served for transformer implementation. Does it makes sense to use it in the extentions catalog? #Resolved
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. I think ML.Net team treats it at a pattern of handling multiple overloaded methods thus not confined to which type of class it can be used. yet it's just a minor comments so feel free to ignore #Resolved
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. Four parameters to a function is not too bad. If you are expecting to enhance this API further in the future by adding more parameters, I would suggest adding an Options parameter now. If there are no plans to add new parameters now, please add an Options parameter the next time there is a new parameter added. E.g. We have now migrated rootCauseThreshold from being a private constant in RootCauseAnaylyzer to being a parameter here. But I also see _anomalyRatioThreshold and _anomalyPreDeltaThreshold as private constants there? Does it make sense to convert them to parameters? If so, please create an Options class and add all those parameters there. In reply to: 438493061 [](ancestors = 438493061)
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 suggestion, have updated #Resolved
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. Sorry, I don't think I communicated well. The pattern in ML.NET is to have two overloaded functions. A convenience function and a full function. The convenience function takes in a minimal set of parameters that will meet the common use case and use default values for all other parameters. We always have this function. The second function is if a particular feature requires a function with lots of parameters. In this case we add a second overloaded function that takes an Options parameter where the user can set each parameter individually. So at the very least you should have one function without an Options parameter. You add a second function with an Options parameter only if you are going to need lots of parameters. Like I mentioned above, if all you need are four parameters, a single function is okay. Only if you were going to migrate the other constants from RootCauseAnalyzer, please add a second function with an Options parameter. In reply to: 442571173 [](ancestors = 442571173) |
||
| { | ||
| IHostEnvironment host = CatalogUtils.GetEnvironment(catalog); | ||
|
|
||
|
|
@@ -205,7 +206,7 @@ public static RootCause LocalizeRootCause(this AnomalyDetectionCatalog catalog, | |
| host.CheckUserArg(beta >= 0 && beta <= 1, nameof(beta), "Must be in [0,1]"); | ||
|
|
||
| //find out the root cause | ||
| RootCauseAnalyzer analyzer = new RootCauseAnalyzer(src, beta); | ||
| RootCauseAnalyzer analyzer = new RootCauseAnalyzer(src, beta, rootCauseThreshold); | ||
| RootCause dst = analyzer.Analyze(); | ||
| return dst; | ||
| } | ||
|
|
||
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.
What is the range of valid values for this parameter? #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.
Have updated the range