Static extension work for time series. by montebhoover · Pull Request #1656 · dotnet/machinelearning

montebhoover · 2018-11-16T22:42:33Z

Static extension work for time series.

montebhoover · 2018-11-26T19:31:59Z

Both SsaChancePointDetector and IidChangePointDetector are complete with working unit tests. I will add the tests for SpikeDetector for SSA and IID shortly. #Resolved

Ivanidzo4ka · 2018-11-26T21:08:22Z

namespace Microsoft.ML.Runtime.TimeSeriesProcessing

Can we change namespace to something not containing Runtime word?
Microsoft.ML.TimeSeries maybe? #WontFix

Refers to: src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs:28 in 8f6eca5. [](commit_id = 8f6eca5, deletion_comment = False)

Ivanidzo4ka · 2018-11-26T21:10:45Z

+        {
+            private readonly int _confidence;
+            private readonly int _changeHistoryLength;
+            private readonly SequentialAnomalyDetectionTransformBase<System.Single, Microsoft.ML.Runtime.TimeSeriesProcessing.IidAnomalyDetectionBase.State>.MartingaleType _martingale;


SequentialAnomalyDetectionTransformBase<System.Single, Microsoft.ML.Runtime.TimeSeriesProcessing.IidAnomalyDetectionBase.State>.MartingaleType [](start = 29, length = 142)

I'm not sure who should change that, but that enum shouldn't depend on that enormous state class definition. (And if I correctly remember it's a different enum for each type you use in generic) #Resolved

I'd be happy to make the change but my powers are not yet great enough to do so. @ganik, would you be able to help with that?

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

montebhoover · 2018-11-26T23:28:40Z

@ganik is added to the review. #Closed

sfilipi · 2018-11-26T23:37:53Z

namespace Microsoft.ML.Runtime.TimeSeriesProcessing

in the other files as well.

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

Refers to: src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs:28 in 8f6eca5. [](commit_id = 8f6eca5, deletion_comment = False)

sfilipi · 2018-11-26T23:41:38Z

+                PipelineColumn[] toOutput,
+                IReadOnlyDictionary<PipelineColumn, string> inputNames,
+                IReadOnlyDictionary<PipelineColumn, string> outputNames,
+                IReadOnlyCollection<string> usedNames)


does this need to get used? #WontFix

The Reconcile method in the base class that we're overriding expects this. Maybe a dumb question, but is there a way to avoid including this as we're overriding the method?

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

no, even if it is not a required param, you still have to include it.

In reply to: 236480124 [](ancestors = 236480124,236470960)

sfilipi · 2018-11-26T23:42:25Z

+                PipelineColumn[] toOutput,
+                IReadOnlyDictionary<PipelineColumn, string> inputNames,
+                IReadOnlyDictionary<PipelineColumn, string> outputNames,
+                IReadOnlyCollection<string> usedNames)


usedNames [](start = 44, length = 9)

same here. #WontFix

sfilipi · 2018-11-26T23:43:47Z

+
+        private sealed class Prediction
+        {
+            // Note that this field must be named "Data"; we ultimately convert


must be named "Data" [](start = 35, length = 21)

would help to add why this is the case #Resolved

The following lines of the comment were an attempt to explain why - is there a way I can explain this better?

The big picture is that eventually we want to look at the result as an enumerable, and since that isn't supported natively in the static API, we convert the resulting dataview to dynamic. Once we have an enumerable in the dynamic API, it requires string-named columns. In the conversion of the static dataview to the dynamic dataview, ML.NET by default names the output column "Data", thus the user must use the field name Data here. It wouldn't be hard to add a parameter to the static extension that allows the user to pick their own name, but @TomFinley was explaining that the static extensions were designed so users do not need to use custom strings.

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

make sense! I think i have seen those be specified in Attributes, rather than names, but this is fine.

In reply to: 236476972 [](ancestors = 236476972,236471499)

sfilipi · 2018-11-26T23:46:27Z

+            {
+                row = enumerator.Current;
+
+                Assert.Equal(expectedValues[index++], row.Data[0]);


Assert.Equal [](start = 16, length = 12)

I tend to use CompareWithTolerance, for the test's stability. #Resolved

Thanks! What namespace is CompareNumbersWithTolerance from? I'm having trouble with it...

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

oh! this class needs to extend the BaseTestClass :)

In reply to: 236484324 [](ancestors = 236484324,236472062)

montebhoover · 2018-11-26T23:55:06Z

@codemzs is added to the review. #Closed

montebhoover · 2018-11-26T23:55:37Z

namespace Microsoft.ML.Runtime.TimeSeriesProcessing

Is is okay if I make a separate PR to change the namespace title? I don't want to interrupt the work that @codemzs is doing on the time series prediction engine.

In reply to: 441841856 [](ancestors = 441841856,441798417)

Refers to: src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs:28 in 8f6eca5. [](commit_id = 8f6eca5, deletion_comment = False)

montebhoover · 2018-11-27T00:07:03Z

@TomFinley is added to the review. #Closed

Ivanidzo4ka

sfilipi

With all the latest changes, I think this PR needs to be reviewed again.

abgoswam

…puMath library.

…h intrinsics.

…lues with intrinsics." This reverts commit 1551a8d.

This reverts commit a7e9767.

…ges in CpuMath library." This reverts commit 9b4ef97.

…ability between Core 3.0 Intrinsics and SSE.

montebhoover requested a review from codemzs November 16, 2018 22:42

sfilipi reviewed Nov 17, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs Outdated

sfilipi reviewed Nov 17, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/SsaSpikeDetector.cs Outdated

Ivanidzo4ka reviewed Nov 19, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/IidChangePointDetector.cs Outdated

montebhoover commented Nov 21, 2018

View reviewed changes

Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesStaticTests.cs Outdated

montebhoover requested review from artidoro and ganik November 21, 2018 01:29

montebhoover commented Nov 21, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/TimeSeriesStatic.cs Outdated

montebhoover commented Nov 21, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/TimeSeriesStatic.cs Outdated

montebhoover changed the title ~~[WIP] Static extension work for time series.~~ Static extension work for time series. Nov 26, 2018

montebhoover commented Nov 26, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/TimeSeriesStatic.cs Outdated

Ivanidzo4ka reviewed Nov 26, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/TimeSeriesStatic.cs Outdated

Ivanidzo4ka reviewed Nov 26, 2018

View reviewed changes

sfilipi reviewed Nov 26, 2018

View reviewed changes

Comment thread src/Microsoft.ML.TimeSeries/TimeSeriesStatic.cs

sfilipi reviewed Nov 26, 2018

View reviewed changes

Comment thread test/Microsoft.ML.TimeSeries.Tests/TimeSeriesStaticTests.cs Outdated

sfilipi reviewed Nov 26, 2018

View reviewed changes

Ivanidzo4ka approved these changes Nov 29, 2018

View reviewed changes

sfilipi previously approved these changes Nov 29, 2018

View reviewed changes

montebhoover requested a review from Anipik December 10, 2018 22:14

montebhoover force-pushed the mohoov/timeseries branch from a73a355 to 2464e04 Compare January 9, 2019 22:28

sfilipi approved these changes Jan 10, 2019

View reviewed changes

shauheen removed request for Anipik and codemzs January 10, 2019 18:12

abgoswam approved these changes Jan 10, 2019

View reviewed changes

montebhoover added 19 commits January 10, 2019 10:20

Initial static extension work for time series.

3f1a7d1

Add IidChangePointDetect static exstension.

fe5281b

Unit test working for IidChangePointDetector.

10e68b8

Moved static extensions from base classes to TimeSeriesStatic.

17c667f

Address comments regarding types, usings, and test precision.

c848b41

Added static extension for IidSpikeDetector.

f653fca

Added static extension for SsaSpikeDetection.

07c3a9f

Fix conflicts with master.

71e11bf

Updated doc comments.

3cc048c

Update expected values at 6th significant digit based on changes in C…

0e52916

…puMath library.

Increase tolerance for change point expected values.

32b4fda

Change precision from 6 to 5 sig digits because of varying values wit…

9cd09b2

…h intrinsics.

Revert "Change precision from 6 to 5 sig digits because of varying va…

3b09025

…lues with intrinsics." This reverts commit 1551a8d.

Revert "Increase tolerance for change point expected values."

87f2d8b

This reverts commit a7e9767.

Revert "Update expected values at 6th significant digit based on chan…

cb03081

…ges in CpuMath library." This reverts commit 9b4ef97.

Update API naming.

7159eb3

Reduce precision from 7 to 6 significant digits for tests due to vari…

4a7e2fe

…ability between Core 3.0 Intrinsics and SSE.

Reduce precision from 6 to 5 significant digits for tests due to vari…

15c7999

…ability between Core 3.0 Intrinsics and SSE.

Use CompareNumbersWithToloerance from BaseTestBaseline.

7cd4bbb

montebhoover force-pushed the mohoov/timeseries branch from 2464e04 to 7cd4bbb Compare January 10, 2019 18:24

montebhoover added 3 commits January 10, 2019 10:54

Create new project Microsoft.ML.TimeSeries.StaticPipe.

c1e3f45

Fix for new NetFX build leg.

576ab4d

Fix typo

a8c6efd

montebhoover merged commit 638d4f4 into dotnet:master Jan 10, 2019

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

Conversation

montebhoover commented Nov 16, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

montebhoover commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Ivanidzo4ka commented Nov 26, 2018 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka Nov 26, 2018 • edited by montebhoover Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montebhoover Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

montebhoover commented Nov 26, 2018

Uh oh!

sfilipi commented Nov 26, 2018

Uh oh!

Uh oh!

sfilipi Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montebhoover Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

sfilipi Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

sfilipi Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montebhoover Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sfilipi Nov 26, 2018 • edited by montebhoover Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montebhoover Nov 27, 2018

Choose a reason for hiding this comment

Uh oh!

sfilipi Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

montebhoover commented Nov 26, 2018

Uh oh!

montebhoover commented Nov 26, 2018

Uh oh!

montebhoover commented Nov 27, 2018

Uh oh!

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Uh oh!

sfilipi left a comment

Choose a reason for hiding this comment

Uh oh!

abgoswam left a comment

Choose a reason for hiding this comment

Uh oh!

montebhoover commented Nov 26, 2018 •

edited

Loading

Ivanidzo4ka commented Nov 26, 2018 •

edited by sfilipi

Loading

Ivanidzo4ka Nov 26, 2018 •

edited by montebhoover

Loading

sfilipi Nov 26, 2018 •

edited

Loading

sfilipi Nov 26, 2018 •

edited

Loading

sfilipi Nov 26, 2018 •

edited

Loading

montebhoover Nov 27, 2018 •

edited

Loading

sfilipi Nov 26, 2018 •

edited by montebhoover

Loading