Port Time Series by codemzs · Pull Request #977 · dotnet/machinelearning

codemzs · 2018-09-21T17:12:36Z

Port of time series prediction.

ExponentialAverage
IidChangePointDetector
IidSpikeDetector
PercentileThresholdTransform
PValueTransform
SlidingWindowTransform
SsaChangePointDetector
SsaSpikeDetector

…to timeseries # Conflicts: # Microsoft.ML.sln

Ivanidzo4ka · 2018-09-21T17:15:53Z

src/Microsoft.ML.TimeSeries/FftUtils.cs

+        }
+
+        private const string DllName = "MklImports.dll";
+        private const string DllProxyName = "MklImports.dll";


You don't need one of them. #Closed

Ivanidzo4ka · 2018-09-21T17:16:13Z

src/Microsoft.ML.TimeSeries/FftUtils.cs

+                    FreeDescriptor(ref descriptor);
+            }
+
+            // REVIEW saamizad: for some reason the native backward scaling for DFTI in MKL does not work.


REVIEW saamizad [](start = 15, length = 15)

Cn you clean code from aliases? #Closed

markusweimer · 2018-09-21T17:21:29Z

This PR doesn't have a useful title, nor does it have a description. Please address this, and the missing Issue # before merging. #Resolved

test/Microsoft.ML.Tests/TimeSeries.cs

codemzs · 2018-09-21T18:11:49Z

Yes, sir. I was planning on doing that! :)

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

…to timeseries

shauheen

Please complete the necessary design documentation on this work first.

src/Microsoft.ML.TimeSeries/FftUtils.cs

Microsoft.ML.sln

Zruty0 · 2018-09-24T21:34:02Z

src/Microsoft.ML.TimeSeries/IidChangePointDetector.cs

@@ -0,0 +1,148 @@
+//------------------------------------------------------------------------------
+// <copyright company="Microsoft Corporation">


Microsoft [](start = 23, length = 9)

fix headers #Closed

Zruty0 · 2018-09-24T21:34:24Z

src/Microsoft.ML.TimeSeries/SequentialAnomalyDetectionTransformBase.cs

+
+namespace Microsoft.ML.Runtime.TimeSeriesProcessing
+{
+    // REVIEW saamizad: This base class and its children classes generate one output column of type VBuffer<Double> to output 3 different anomaly scores as well as


saamizad [](start = 14, length = 8)

remove aliases #Resolved

Keep the 'REVIEW' though

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

Zruty0

🕐

…to timeseries

eerhardt · 2018-10-02T19:34:07Z

pkg/Microsoft.ML.TimeSeries/Microsoft.ML.TimeSeries.nupkgproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Include="$(PackagesDir)\mlnetmkldeps\$(MlNetMklDepsPackageVersion)\LICENSE.txt" Pack="true" PackagePath=".\NOTICE.txt" /> 


I don't think we need the license file here since we are not packaging MKL in this nuget. #Resolved

eerhardt · 2018-10-02T19:37:17Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

        {
+            static Native()
+            {
+                var t = ErrorMessage(0);


You can use:

_ = ErrorMessage(0);

instead of var t. _ signifies "I want to capture the variable, but I don't actually use the variable". #Resolved

eerhardt · 2018-10-02T19:39:27Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

        {
+            static Native()
+            {
+                var t = ErrorMessage(0);


Let's add the reasoning/comment here on why we are doing this. #Resolved

eerhardt · 2018-10-02T19:44:10Z

src/Native/SymSgdNative/CMakeLists.txt

 target_link_libraries(SymSgdNative PUBLIC ${MKL_LIBRARY})

+if(APPLE)
+    set_target_properties(SymSgdNative PROPERTIES INSTALL_RPATH "@loader_path;@loader_path/../../../../../${MKL_LIB_RPATH};${MKL_LIB_PATH}")


${MKL_LIB_PATH} shouldn't be necessary, right? When we are running in our repo, these assemblies should live together, so @loader_path should just take care of it. #Closed

eerhardt · 2018-10-02T19:44:28Z

src/Native/MklProxyNative/CMakeLists.txt

+target_link_libraries(MklProxyNative PUBLIC ${MKL_LIBRARY})
+
+if(APPLE)
+    set_target_properties(MklProxyNative PROPERTIES INSTALL_RPATH "@loader_path;${MKL_LIB_PATH}")


We shouldn't need MKL_LIB_PATH here, right? MklProxyNative will always be in the same directory as MklImports. #Closed

eerhardt · 2018-10-02T19:46:08Z

src/Native/build.proj

    <ItemGroup>
-      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)MklImports$(NativeLibExtension)"
-                          RelativePath="Microsoft.ML.HalLearners\runtimes\$(PackageRid)\native" /> 
+      <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)MklProxyNative$(NativeLibExtension)"


This line needs to go above the previous ItemGroup. The reason this ItemGroup exists down here is because the one above adds the symbols for the assemblies we produce in this repo. Since MklImports is not produced by this repo, we have no symbols for it. But MklProxyNative IS produced by this repo, so we need to package the $(NativeLibSymbolExtension) files as well. #Closed

eerhardt · 2018-10-02T19:47:11Z

src/Native/build.proj


    <PropertyGroup>
-      <BuildArgs>$(Configuration) $(TargetArchitecture) --mkllibpath $(PackagesDir)mlnetmkldeps\$(MlNetMklDepsPackageVersion)\runtimes\$(PackageRid)\native</BuildArgs>
+      <BuildArgs>$(Configuration) $(TargetArchitecture) --mkllibpath $(PackagesDir)mlnetmkldeps\$(MlNetMklDepsPackageVersion)\runtimes\$(PackageRid)\native --mkllibrpath mlnetmkldeps/$(MlNetMklDepsPackageVersion)/runtimes/$(PackageRid)/native</BuildArgs>


There's no such thing as 'rpath' on Windows, right? We shouldn't need to use/pass it on Windows. #Closed

eerhardt · 2018-10-02T19:47:29Z

src/Native/build.sh

+        --mkllibrpath)
+            shift
+            __mkllibrpath=$1
+            ;;			


(nit) tabs at the end of this line #Resolved

eerhardt · 2018-10-02T19:48:49Z

src/Native/MklProxyNative/MklProxyNative.cpp

+    CceFormat = 57        /* Complex conjugate-even */
+};
+
+EXPORT_API(int) DftiSetValue(void *handle, ConfigParam config_param, ...);


It would be nice having a comment here explaining why we need to make these "proxy" methods. #Closed

eerhardt · 2018-10-02T19:50:24Z

src/Native/SymSgdNative/CMakeLists.txt

 target_link_libraries(SymSgdNative PUBLIC ${MKL_LIBRARY})

+if(APPLE)
+    set_target_properties(SymSgdNative PROPERTIES INSTALL_RPATH "@loader_path;@loader_path/../../../../../${MKL_LIB_RPATH};${MKL_LIB_PATH}")


@loader_path/../../../../../${MKL_LIB_RPATH} [](start = 78, length = 44)

How about let's put the ../../.. outside of the CMake files as well? The whole relative path should be passed into ${MKL_LIB_RPATH}, so here we can just say:

@loader_path/${MKL_LIB_RPATH} #Closed

eerhardt · 2018-10-02T19:53:21Z

pkg/Microsoft.ML.TimeSeries/Microsoft.ML.TimeSeries.nupkgproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Content Include="..\common\CommonPackage.props" Pack="true" PackagePath="build\netstandard2.0\$(MSBuildProjectName).props" />


TimeSeries doesn't ship any native assemblies, so it doesn't need this .props file. #Resolved

codemzs · 2018-10-02T20:39:37Z

    [Fact(Skip = "TEMP: Missing MKL Binary.")]

We need a real dataset for this. Opened issue #1120

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

Refers to: test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs:3753 in be7ccff. [](commit_id = be7ccff, deletion_comment = False)

…to timeseries # Conflicts: # Microsoft.ML.sln

pkg/Microsoft.ML.Mkl.Redist/Microsoft.ML.Mkl.Redist.nupkgproj

pkg/Microsoft.ML.TimeSeries/Microsoft.ML.TimeSeries.nupkgproj

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

src/Native/build.proj

eerhardt · 2018-10-02T21:55:22Z

    [Fact(Skip = "TEMP: Missing MKL Binary.")]

(minor) Can we add that issue link into the code?

In reply to: 426421800 [](ancestors = 426421800,426390719)

Refers to: test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs:3753 in be7ccff. [](commit_id = be7ccff, deletion_comment = False)

eerhardt

shauheen

shauheen

codemzs added 3 commits September 21, 2018 10:02

time series.

e6d264b

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

90f9cbe

…to timeseries # Conflicts: # Microsoft.ML.sln

fixes.

393b1e0

codemzs requested review from montebhoover and yaeldekel September 21, 2018 17:12

Ivanidzo4ka reviewed Sep 21, 2018

View reviewed changes

test/Microsoft.ML.Tests/TimeSeries.cs Outdated Show resolved Hide resolved

codemzs added 3 commits September 21, 2018 11:14

more tests and PR feedback.

b4e34a5

cleanup.

bf3e1f7

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

14dc307

…to timeseries

codemzs requested a review from Zruty0 September 21, 2018 18:38

codemzs changed the title ~~Timeseries~~ Time Series Sep 21, 2018

shauheen self-requested a review September 21, 2018 18:40

shauheen suggested changes Sep 21, 2018

View reviewed changes

fix linux/macOS test failures.

da4d0ed

Ivanidzo4ka reviewed Sep 21, 2018

View reviewed changes

src/Microsoft.ML.TimeSeries/FftUtils.cs Show resolved Hide resolved

Zruty0 reviewed Sep 24, 2018

View reviewed changes

Microsoft.ML.sln Show resolved Hide resolved

Zruty0 reviewed Sep 24, 2018

View reviewed changes

Zruty0 suggested changes Sep 24, 2018

View reviewed changes

codemzs added 2 commits September 24, 2018 19:03

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

13af732

…to timeseries

PR feedback.

f9d4ad8

codemzs changed the title ~~Time Series~~ Port Time Series Sep 25, 2018

build break.

948cc04

codemzs changed the title ~~Port Time Series~~ WIP: Port Time Series Sep 25, 2018

justinormont mentioned this pull request Sep 25, 2018

Rolling Cross-validation for Time-series #1026

Open

Add MKL proxy.

acadfa2