Estimators for Timeseries SSA / IID ChangepointDetection and SpikeDetection transforms #1254

ganik · 2018-10-13T23:43:41Z

Created Estimators for SSAChangePointDetector, SSASpikeDetector, IidChangePointDetector, IidSpikeDetector
Added unit tests

Merge

…& IID SpikeDetection transformes. Added unit tests

Ivanidzo4ka · 2018-10-16T00:01:51Z

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

+            /// <summary>
+            /// Counts the number of rows observed by the transform so far.
+            /// </summary>
+            protected int RowCounter { get; private set; }


int [](start = 22, length = 3)

should be long, we support more than 2billion rows in dataview #Resolved

Ivanidzo4ka · 2018-10-16T00:03:14Z

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesEstimatorTests.cs

+using Microsoft.ML.Runtime.RunTests;
+using System;
+using System.Collections.Generic;
+using Microsoft.ML.Runtime.TimeSeriesProcessing;


ctrl+r+g #Resolved

Ivanidzo4ka · 2018-10-16T00:04:11Z

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesEstimatorTests.cs

+
+            var pipe = new SsaChangePointEstimator(Env, 
+                Confidence, ChangeHistorySize, MaxTrainingSize, SeasonalitySize, "Value", "Change");
+            TestEstimatorCore(pipe, dataView);


TestEstimatorCore [](start = 12, length = 17)

can we have at least one test with wrong schema? #Resolved

good suggestion, found few issues when added the invalid input tests

In reply to: 225355511 [](ancestors = 225355511)

Ivanidzo4ka · 2018-10-16T00:05:20Z

test/BaselineOutput/SingleDebug/SavePipe/SavePipeIidChangePoint-Schema.txt

@@ -16,7 +16,7 @@
  fAnomaly: Vec<R4, 4>
    Metadata 'SlotNames': Vec<Text, 4>: Length=4, Count=4
      [0] 'Alert', [1] 'Raw Score', [2] 'P-Value Score', [3] 'Martingale Score'
---- IidChangePointDetector ----
+---- RowToRowMapperTransform ----


You can move this tests to Common/SavePipe folder, or you need to do same thing in SingleRelease/SavePipe #Resolved

made changes in SingleRelease too

In reply to: 225355661 [](ancestors = 225355661)

Ivanidzo4ka · 2018-10-16T00:07:32Z

src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs

+            string output)
+        {
+            Contracts.CheckValue(env, nameof(env));
+            _host = env.Register("SsaSpikeEstimator");


"SsaSpikeEstimator" [](start = 33, length = 19)

nameof(SsaSpikeEstimator) #Closed

Ivanidzo4ka · 2018-10-16T00:07:55Z

src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs

+        private readonly int _trainingWindowSize;
+        private readonly int _seasonalityWindowSize;
+
+        public SsaSpikeEstimator(


SsaSpikeEstimator [](start = 15, length = 17)

Public constructors need to have proper comments. #Closed

Ivanidzo4ka · 2018-10-16T00:10:23Z

src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs

+        {
+            public PipelineColumn Input { get; }
+
+            public OutColumn(Vector<float> input,


OutColumn [](start = 19, length = 9)

Did you put static method which calls this OutColumn in other class, or it just missing? #Resolved

removed pigsty extensions, will be part of separate PR

In reply to: 225356515 [](ancestors = 225356515)

Ivanidzo4ka · 2018-10-16T00:11:14Z

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesEstimatorTests.cs

+        }
+
+        [Fact]
+        void TestSsaSpikeEstimator()


TestSsaSpikeEstimator [](start = 13, length = 21)

Can you add pigsty extension tests to StaticPipeTests.cs file? (see LpGcNormAndWhitening as example) #Closed

removed pigsty extensions, will be part of separate PR

In reply to: 225356643 [](ancestors = 225356643)

Ivanidzo4ka · 2018-10-16T00:12:30Z

src/Microsoft.ML.TimeSeries/SsaChangePointDetector.cs

+            string output)
+        {
+            Contracts.CheckValue(env, nameof(env));
+            _host = env.Register("SsaChangePointEstimator");


"SsaChangePointEstimator" [](start = 33, length = 25)

nameof(SsaChangePointEstimator) #Closed

Ivanidzo4ka · 2018-10-16T00:17:33Z

src/Microsoft.ML.TimeSeries/SequentialAnomalyDetectionTransformBase.cs

+                _slotNames[0] = "Alert";
+                _slotNames[1] = "Raw Score";
+                _slotNames[2] = "P-Value Score";
+                _slotNames[3] = "Martingale Score";


I would simplify and moved to _slotName definition. #Closed

tried but getting build error: "... a field initialiser outside the constructor"

In reply to: 225357735 [](ancestors = 225357735)

I think Ivan meant slotNames = new[] { "Alert", "Foo", "Bar", "Baz" }

In reply to: 226489440 [](ancestors = 226489440,225357735)

yep, tried this and got build error.

In reply to: 226785830 [](ancestors = 226785830,226489440,225357735)

oh u mean inside the constructor. ok

In reply to: 226806798 [](ancestors = 226806798,226785830,226489440,225357735)

Ivanidzo4ka · 2018-10-16T00:25:59Z

src/Microsoft.ML.TimeSeries/IidSpikeDetector.cs

        {
-            return new IidSpikeDetector(env, this, newSource);
+            Contracts.CheckValue(env, nameof(env));
+            _host = env.Register("IidSpikeEstimator");


"IidSpikeEstimator" [](start = 33, length = 19)

nameof #Resolved

Ivanidzo4ka · 2018-10-16T00:26:24Z

src/Microsoft.ML.TimeSeries/IidSpikeDetector.cs

+        private readonly int _confidence;
+        private readonly int _pvalueHistoryLength;
+
+        public IidSpikeEstimator(


add Summary #Resolved

Ivanidzo4ka · 2018-10-16T00:26:48Z

src/Microsoft.ML.TimeSeries/IidChangePointDetector.cs

        {
-            return new IidChangePointDetector(env, this, newSource);
+            Contracts.CheckValue(env, nameof(env));
+            _host = env.Register("IidChangePointEstimator");


IidChangePointEstimator [](start = 34, length = 23)

nameof() #Resolved

Ivanidzo4ka · 2018-10-16T00:26:54Z

src/Microsoft.ML.TimeSeries/IidChangePointDetector.cs

+        private readonly int _confidence;
+        private readonly int _changeHistoryLength;
+
+        public IidChangePointEstimator(


add summary comment #Resolved

Merge

Zruty0 · 2018-10-17T21:42:03Z

src/Microsoft.ML.TimeSeries/IidChangePointDetector.cs

-        public IidChangePointDetector(IHostEnvironment env, Arguments args, IDataView input)
-            : base(new BaseArguments(args), LoaderSignature, env, input)
+        // Factory method for SignatureDataTransform.
+        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


public [](start = 8, length = 6)

make it private #Closed

Merge

TomFinley · 2018-10-30T18:31:13Z

            verWeCanReadBack: 0x00010001,

You appear to have changed the format. Have you?

If you have, please handle backwards compatibility correctly, thanks. #Closed

Refers to: src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs:226 in 48b0ddd. [](commit_id = 48b0ddd, deletion_comment = False)

TomFinley · 2018-10-30T18:33:04Z

        // bool: ShouldComputeForecastIntervals

I do not see that the load comments match the save comments. #Closed

Refers to: src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs:372 in 48b0ddd. [](commit_id = 48b0ddd, deletion_comment = False)

TomFinley · 2018-10-30T18:51:04Z

    public void ChangePointDetectionWithSeasonality()

I guess you didn't write this test, but it takes a comparatively long time at 14 seconds (debug build), and my observation is that my machine is some multiple faster than the build machines. It appears to also be failing. While you're fixing it, maybe tone it down a bit so it hammers the build systems a bit less. #Closed

Refers to: test/Microsoft.ML.TimeSeries.Tests/TimeSeriesDirectApi.cs:80 in 48b0ddd. [](commit_id = 48b0ddd, deletion_comment = False)

TomFinley

Thanks for working on this @ganik ! I have two major concerns:

You broke backwards compatibility,
You have added two additional tests that at least on Windows debug each take about two minutes, which on the build server I'd expect to take even longer. Whether a fault of the test or the code, this is not something we want in our system.

TomFinley · 2018-10-30T18:53:14Z

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesEstimatorTests.cs

+        }
+
+        [Fact]
+        void TestSsaChangePointEstimator()


So...

That's a long time for a test to run, and this isn't even on the slower build machines, but on my dev box here, which is comparatively fast. Could you tone it down a bit? #Closed

TomFinley · 2018-10-30T18:55:27Z

test/Microsoft.ML.TimeSeries.Tests/TimeSeriesEstimatorTests.cs

+        }
+
+        [Fact]
+        void TestSsaSpikeEstimator()


Another two minute test. If this is expected given the large volume of data, please tone down the volume. Certainly we don't need a 150k sample for a test. 😛 If unexpected, please investigat.e #Closed

Zruty0 · 2018-10-30T20:32:25Z

src/Microsoft.ML.TimeSeries/SequentialTransformerBase.cs

+        /// <param name="env">A reference to the environment variable.</param>
+        /// <param name="outputColType"></param>
+        protected SequentialTransformerBase(int windowSize, int initialWindowSize, string inputColumnName, string outputColumnName,
+            string name, IHostEnvironment env, ColumnType outputColType)


IHostEnvironment [](start = 25, length = 16)

This is always the first parameter in all our ctors. Also, the 'env plus name' pattern is uncommon: we tend to just take IHost in those protected ctors #Resolved

Zruty0 · 2018-10-30T20:40:01Z

src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs

-                verWrittenCur: 0x00010001, // Initial
-                verReadableCur: 0x00010001,
+                //verWrittenCur: 0x00010001, // Initial
+                verWrittenCur: 0x00010002, // Added saving _state and _nextPrediction


saving _state [](start = 52, length = 13)

Why do you need to do it now? Is this for the stateful row and spawning off new models? #Closed

So the preexisting test TestEstimatorCore() will pass on new timeseries estimators. One of the test steps in TestEstimatorCore() is to save and re-load transformer from disk. Without this fix saving and reloading of timeseries SSA transformers changes its prediction due to lost _state and _nextPrediction

In reply to: 229477366 [](ancestors = 229477366)

Zruty0

TomFinley

Thanks @ganik !

Merge

ganik and others added 3 commits October 10, 2018 14:49

Merge pull request #1 from dotnet/master

f5e421e

Merge

Made estimators for Timeseries SSA and IID ChangePointDetection, SSA …

facc77f

…& IID SpikeDetection transformes. Added unit tests

Add more unit tests

a586e83

ganik requested review from Ivanidzo4ka, codemzs and montebhoover October 13, 2018 23:43

ganik added 2 commits October 15, 2018 13:14

fix tests

477f4eb

fix unit tests

6b1b119

Ivanidzo4ka reviewed Oct 16, 2018

View reviewed changes

fix tests

8647659

Ivanidzo4ka reviewed Oct 16, 2018

View reviewed changes

ganik and others added 6 commits October 15, 2018 19:23

Merge pull request #2 from dotnet/master

13f5f76

Merge

Merge branch 'master' into ganik/ssa

db25cb9

fix build

e9c2d15

fix comments

da77379

fix build

ee97924

fix unit tests

a4853ec

Zruty0 reviewed Oct 17, 2018

View reviewed changes

ganik and others added 12 commits October 28, 2018 16:34

indent

3fa9f95

indent

e140152

Merge pull request #3 from dotnet/master

1aa7f55

Merge

Merge branch 'master' into ganik/ssa

1ccd234

fix tests

721e04f

fix tests

6f92e4d

fix tests

1b3c263

enable tests back

3f1f478

make Mapper private. Implement ITransformTemplate

28cc331

Batch prediction test

a6a03c7

fix comments

65108fd

fix test

48b0ddd

TomFinley suggested changes Oct 30, 2018

View reviewed changes

ganik added 2 commits October 30, 2018 12:34

fix comments

78a0d72

fix backward compat

58622f8

Zruty0 reviewed Oct 30, 2018

View reviewed changes

Zruty0 approved these changes Oct 30, 2018

View reviewed changes

TomFinley approved these changes Oct 30, 2018

View reviewed changes

ganik and others added 4 commits October 30, 2018 13:53

fix comments

ba876d2

fix comments

8320f24

Merge pull request #4 from dotnet/master

546f2fa

Merge

Merge branch 'master' into ganik/ssa

c5c262e

shauheen merged commit 1391107 into dotnet:master Oct 31, 2018

ganik deleted the ganik/ssa branch November 2, 2018 00:03

TomFinley mentioned this pull request Nov 26, 2018

Added Feature Contribution Calculation Transform #1677

Merged

ghost locked as resolved and limited conversation to collaborators Mar 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Estimators for Timeseries SSA / IID ChangepointDetection and SpikeDetection transforms #1254

Estimators for Timeseries SSA / IID ChangepointDetection and SpikeDetection transforms #1254

ganik commented Oct 13, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

ganik Oct 19, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

ganik Oct 18, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

ganik Oct 18, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

ganik Oct 18, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

ganik Oct 18, 2018

Zruty0 Oct 19, 2018

ganik Oct 19, 2018

ganik Oct 19, 2018

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley left a comment

TomFinley Oct 30, 2018 •

edited

Loading

TomFinley Oct 30, 2018 •

edited

Loading

Zruty0 Oct 30, 2018 •

edited by ganik

Loading

Zruty0 Oct 30, 2018 •

edited

Loading

ganik Oct 30, 2018

Zruty0 left a comment

TomFinley left a comment

Estimators for Timeseries SSA / IID ChangepointDetection and SpikeDetection transforms #1254

Estimators for Timeseries SSA / IID ChangepointDetection and SpikeDetection transforms #1254

Conversation

ganik commented Oct 13, 2018

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley commented Oct 30, 2018 • edited Loading

TomFinley commented Oct 30, 2018 • edited Loading

TomFinley commented Oct 30, 2018 • edited Loading

TomFinley left a comment

Choose a reason for hiding this comment

TomFinley Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 30, 2018 • edited by ganik Loading

Choose a reason for hiding this comment

Zruty0 Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Ivanidzo4ka Oct 16, 2018 •

edited by ganik

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley commented Oct 30, 2018 •

edited

Loading

TomFinley Oct 30, 2018 •

edited

Loading

TomFinley Oct 30, 2018 •

edited

Loading

Zruty0 Oct 30, 2018 •

edited by ganik

Loading

Zruty0 Oct 30, 2018 •

edited

Loading