-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Time series samples and documentation alignment #2900
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
sfilipi
commented
Mar 9, 2019
- All overloads have method and parameter documentation according to XML template table below.
- The returned estimator and its class hierarchy have documentation according to template table below.
- The corresponding options class and its class hierarchy have documentation
- All overloaded versions have samples
- Sample names are <API_method>.cs or <API_method>WithOptions.cs.
- Sample path mirrors the MLContext path. (e.g. MLContext.Transforms.Categorical.OneHotEncoding goes to Dynamic/Transforms/Categorical/OneHotEncoding.cs
- Sample is included in the API xml documentation.
@@ -111,7 +111,7 @@ public static void IidChangePointDetectorPrediction() | |||
string inputColumnName = nameof(IidChangePointData.Value); | |||
|
|||
// Time Series model. | |||
ITransformer model = ml.Transforms.IidChangePointEstimator(outputColumnName, inputColumnName, 95, Size / 4).Fit(dataView); | |||
ITransformer model = ml.Transforms.DetectIndependentIdenticallyDistributedChangePoint(outputColumnName, inputColumnName, 95, Size / 4).Fit(dataView); |
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.
@CESARDELATORRE, @eerhardt, @TomFinley @shauheen how do you feel about those API names?
I would rather leave it to DetectIIDChangePoint
, but abbreviations have been deemed cryptic. #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 would stick with IID, it is fairly well known abreviation. #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.
We have Lda, Pca, and so on. I feel Iid
is not much worse than them. #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.
Codecov Report
@@ Coverage Diff @@
## master #2900 +/- ##
=========================================
Coverage ? 72.25%
=========================================
Files ? 796
Lines ? 142304
Branches ? 16049
=========================================
Hits ? 102828
Misses ? 35099
Partials ? 4377
|
int confidence, int changeHistoryLength, MartingaleType martingale = MartingaleType.Power, double eps = 0.1) | ||
=> new IidChangePointEstimator(CatalogUtils.GetEnvironment(catalog), outputColumnName, confidence, changeHistoryLength, inputColumnName, martingale, eps); | ||
|
||
/// <summary> | ||
/// Create a new instance of <see cref="IidSpikeEstimator"/> | ||
/// Create a new instance of <see cref="IidSpikeEstimator"/> that detects a spike in an indiependent identically distributed time series. |
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.
indiependent [](start = 96, length = 12)
typo #Resolved
@@ -10,7 +10,8 @@ namespace Microsoft.ML | |||
public static class TimeSeriesCatalog | |||
{ | |||
/// <summary> | |||
/// Create a new instance of <see cref="IidChangePointEstimator"/> | |||
/// Create a new instance of <see cref="IidChangePointEstimator"/> that detects a change of in a indiependent identically distributed time series. |
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.
indiependent [](start = 105, length = 12)
same typo #Resolved
@@ -192,16 +192,9 @@ private static IRowMapper Create(IHostEnvironment env, ModelLoadContext ctx, Dat | |||
} | |||
|
|||
/// <summary> | |||
/// Estimator for <see cref="IidChangePointDetector"/> | |||
/// The <see cref="IEstimator{ITransformer}"/> for detecting a signal change on an Independent Identically Distributed time series. |
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.
IEstimator{ITransformer} [](start = 23, length = 24)
Why this changed? #Closed
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're standardizing the documentation across types, and i added a bit more info too.
In reply to: 264467232 [](ancestors = 264467232)
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.
@shmoradims, @rogancarr @zeahmed when you get a chance. #Resolved |
@@ -30,7 +30,7 @@ public IidChangePointData(float value) | |||
|
|||
// This example creates a time series (list of Data with the i-th element corresponding to the i-th time slot). | |||
// IidChangePointDetector is applied then to identify points where data distribution changed. |
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.
IidChangePointDetector [](start = 11, length = 22)
let's not put class names in the comments. This has already changed and is incorrect now. we can't catch these. #Closed
@@ -55,7 +55,7 @@ public static void SsaSpikeDetectorTransform() | |||
var outputColumnName = nameof(SsaSpikePrediction.Prediction); | |||
|
|||
// The transformed data. | |||
var transformedData = ml.Transforms.SsaSpikeEstimator(outputColumnName, inputColumnName, 95, 8, TrainingSize, SeasonalitySize + 1).Fit(dataView).Transform(dataView); | |||
var transformedData = ml.Transforms.SsaDetectSpike(outputColumnName, inputColumnName, 95, 8, TrainingSize, SeasonalitySize + 1).Fit(dataView).Transform(dataView); |
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.
SsaDetectSpike [](start = 48, length = 14)
Why not DetectSsaSpike instead? I think IID and SSA should be added to the name in the same way. right now we have DetectIidSpike and SsaDetectSpike. #Closed
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 thought of that, but that doesn't make complete sense to me.
IID is the type of distribution.
SSA is a method.
Detect Independent Identically Distributed Spike/Change
makes sense.
and
Singular Spectrum Analysis Detect Spike/Change
makes sense but not Detect Singular Spectrum Analysis Spike
In reply to: 265263423 [](ancestors = 265263423)
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.
Just a random idea and not a required change. How about DetectSpikeBySsa
?
In reply to: 265345768 [](ancestors = 265345768,265263423)
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.
@@ -48,7 +54,7 @@ public static void SsaChangePointDetectorTransform() | |||
var outputColumnName = nameof(ChangePointPrediction.Prediction); | |||
|
|||
// The transformed data. | |||
var transformedData = ml.Transforms.SsaChangePointEstimator(outputColumnName, inputColumnName, 95, 8, TrainingSize, SeasonalitySize + 1).Fit(dataView).Transform(dataView); | |||
var transformedData = ml.Transforms.SsaDetectChangePoint(outputColumnName, inputColumnName, 95, 8, TrainingSize, SeasonalitySize + 1).Fit(dataView).Transform(dataView); |
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.
SsaDetectChangePoint [](start = 48, length = 20)
ditto: DetectSsaChangePoint #Closed
@@ -10,7 +10,8 @@ namespace Microsoft.ML | |||
public static class TimeSeriesCatalog | |||
{ | |||
/// <summary> | |||
/// Create a new instance of <see cref="IidChangePointEstimator"/> | |||
/// Create a new instance of <see cref="IidChangePointEstimator"/> that detects a change of in a independent identically distributed time series. |
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 = 140, length = 1)
add acronym (IID) here so that people know what IID is. #Closed
int confidence, int pvalueHistoryLength, AnomalySide side = AnomalySide.TwoSided) | ||
=> new IidSpikeEstimator(CatalogUtils.GetEnvironment(catalog), outputColumnName, confidence, pvalueHistoryLength, inputColumnName, side); | ||
|
||
/// <summary> | ||
/// Create a new instance of <see cref="SsaChangePointEstimator"/> | ||
/// Create a new instance of <see cref="SsaChangePointEstimator"/> for detecting a change in a time series signal using Singular Spectrum Analysis. |
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 = 154, length = 1)
ditto: add (SSA) #Closed
@@ -26,8 +26,7 @@ | |||
namespace Microsoft.ML.Transforms.TimeSeries | |||
{ |
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.
please also clean up the xml for the 4 estimator classes: SsaSpikeEstimator, etc. Their xml shouldn't have any samples and just cref to extension method APIs for usage, plus include algorithm details, in this case link to SSA paper or IID wikipage. #Closed
{ | ||
[VectorType(4)] | ||
public double[] Prediction { get; set; } | ||
} |
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 like it. #Resolved
SsaDetectSpike or DetectSpikeBySsa? Please also double-check if static APIs' names are aligned with their dynamic ones. #Resolved Refers to: src/Microsoft.ML.TimeSeries.StaticPipe/TimeSeriesStatic.cs:300 in e2487bf. [](commit_id = e2487bf, deletion_comment = False) |
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 a conditional approval. It's valid only if (1) there is only one Example()
per sample file (2) equivalent dynamic and static APIs have the same name.
I removed it. The code is in the history of the file, and should get moved to the machinelearning-samples repository In reply to: 472661246 [](ancestors = 472661246) Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/IidSpikeDetectorTransform.cs:79 in e2487bf. [](commit_id = e2487bf, deletion_comment = False) |
|
||
namespace Microsoft.ML.Samples.Dynamic | ||
{ | ||
public static class SsaDetectChangePoint |
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.
SsaDetectChangePoint [](start = 24, length = 20)
please rename these to match the new API name #Resolved
|
||
namespace Microsoft.ML.Samples.Dynamic | ||
{ | ||
public static class SsaDetectSpike |
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.
SsaDetectSpike [](start = 24, length = 14)
ditto: rename to DetectSpikeBySsa. Make sure to update the filename inside xml CDATA. #Resolved
/// <example> | ||
/// <format type="text/markdown"> | ||
/// <] |
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.
SsaDetectChangePoint [](start = 133, length = 20)
update #Closed
/// <example> | ||
/// <format type="text/markdown"> | ||
/// <] |
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.
SsaDetectSpike [](start = 145, length = 14)
update #Closed
@@ -202,16 +201,8 @@ private static IRowMapper Create(IHostEnvironment env, ModelLoadContext ctx, Dat | |||
} | |||
|
|||
/// <summary> | |||
/// Estimator for <see cref="SsaChangePointDetector"/> | |||
/// The <see cref="IEstimator{ITransformer}"/> for detecting a signal change through <a href="http://arxiv.org/pdf/1206.6910.pdf">Singular Spectrum Analysis (SSA).</a> of time series. |
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 = 166, length = 1)
extra dot #Closed
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.