-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 deseasonality in SrCnnEntireAnomalyDetect #5202
Conversation
Removed this file. #Resolved |
Removed this file #Resolved |
@harishsk is added to the review. #Closed |
@conhua is added to the review. #Closed |
@jinhua is added to the review. #Closed |
src/Microsoft.ML.TimeSeries/PolynomialModel/AbstractPolynomialModel.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ML.TimeSeries/PolynomialModel/AbstractPolynomialModel.cs
Outdated
Show resolved
Hide resolved
|
||
Contracts.CheckParam(weights.Count == _length, nameof(weights)); | ||
if (weights.Count != _length) | ||
throw Contracts.Except("the weight vector is not equal length to the data points"); |
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.
Suggestion: "The length of the weight vector is not equal to the length of the data points." #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.
I updated a description for this PR in the first comment. In reply to: 646231552 [](ancestors = 646231552) |
else // if (deseasonalityMode == SrCnnDeseasonalityMode.Median) | ||
{ | ||
_deseasonalityFunction = new MedianDeseasonality(); | ||
} |
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.
This can be changed to a switch
statement. #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.
In the default case, instead of writing deseasonalityMode == SrCnnDeseasonalityMode.Median
you can add
Contracts.Assert(deseasonalityMode == SrCnnDeseasonalityMode.Median);
(that way, if in the future a value is added to the SrCnnDeseasonalityMode
enum, it will be easier to detect that this code needs to be updated).
In reply to: 443404193 [](ancestors = 443404193)
for (int i = 0; i < values.Length; ++i) | ||
{ | ||
results[i][3] = values[i] - residual[i]; | ||
} |
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.
Can you call GetExpectedValuePeriod
instead? #Resolved
[Theory, CombinatorialData] | ||
public void TestSrCnnAnomalyDetectorWithSeasonalData( | ||
[CombinatorialValues(SrCnnDeseasonalityMode.Stl, SrCnnDeseasonalityMode.Mean, SrCnnDeseasonalityMode.Median)] SrCnnDeseasonalityMode mode | ||
) |
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.
) [](start = 8, length = 1)
nit: move this to the line above. #Resolved
[CombinatorialValues(SrCnnDeseasonalityMode.Stl, SrCnnDeseasonalityMode.Mean, SrCnnDeseasonalityMode.Median)] SrCnnDeseasonalityMode mode | ||
) | ||
{ | ||
var ml = new MLContext(1); |
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.
MLContext [](start = 25, length = 9)
The base class has an MLContext
field. #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.
Could you point out the MLContext
field? As I find in this class, other tests are creating MLContext
by themselves.
In reply to: 443415149 [](ancestors = 443415149)
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.
Sorry, I was wrong. The MLContext
is not in BaseTestClass
, it is in BaseTestBaseline
, which is derived from BaseTestClass
.
In reply to: 443532513 [](ancestors = 443532513,443415149)
[CombinatorialValues(SrCnnDeseasonalityMode.Stl, SrCnnDeseasonalityMode.Mean, SrCnnDeseasonalityMode.Median)] SrCnnDeseasonalityMode mode | ||
) | ||
{ | ||
var ml = new MLContext(1); |
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.
MLContext [](start = 25, length = 9)
Use the MLContext
of the base class. #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.
Same as above comment, the base class don't have this field.
In reply to: 443415470 [](ancestors = 443415470)
} | ||
|
||
/// <summary> | ||
/// The estimated y values. This is the very cool smoothing method. |
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.
This is the very cool smoothing method. [](start = 36, length = 39)
What does this comment mean? #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.
Removed, original, it's refer to loess(local weighted regression)
In reply to: 443960923 [](ancestors = 443960923)
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 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.
We are excited to review your PR.
So we can do the best job, please check:
Fixes #nnnn
in your description to cause GitHub to automatically close the issue(s) when your PR is merged.Description
In time series data, seasonality is the presence of variations that occur at specific regular intervals less than a year, such as weekly, monthly, or quarterly. In time series anomaly detection, it is essential to remove the effect of seasonality before detecting anomalies.
In this PR, we add the de-seasonality process in SrCnnEntireAnomalyDetect.
In the
options
parameter, ifPeriod
is set to a positive integer, de-seasonality will be enabled before detection.Three de-seasonality methods are introduced.
Changes in this PR includes.
SrCnnEntireAnomalyDetector
class that accepts an options class as parameter.TimeSeriesDirectApi
file.