Port Time Series #977
Port Time Series #977
Conversation
…to timeseries # Conflicts: # Microsoft.ML.sln
| } | ||
|
|
||
| private const string DllName = "MklImports.dll"; | ||
| private const string DllProxyName = "MklImports.dll"; |
There was a problem hiding this comment.
You don't need one of them. #Closed
| FreeDescriptor(ref descriptor); | ||
| } | ||
|
|
||
| // REVIEW saamizad: for some reason the native backward scaling for DFTI in MKL does not work. |
There was a problem hiding this comment.
REVIEW saamizad [](start = 15, length = 15)
Cn you clean code from aliases? #Closed
|
This PR doesn't have a useful title, nor does it have a description. Please address this, and the missing Issue # before merging. #Resolved |
|
Yes, sir. I was planning on doing that! :) In reply to: 423611446 [](ancestors = 423611446) |
| @@ -0,0 +1,148 @@ | |||
| //------------------------------------------------------------------------------ | |||
| // <copyright company="Microsoft Corporation"> | |||
There was a problem hiding this comment.
Microsoft [](start = 23, length = 9)
fix headers #Closed
|
|
||
| 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 |
There was a problem hiding this comment.
saamizad [](start = 14, length = 8)
remove aliases #Resolved
There was a problem hiding this comment.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Content Include="$(PackagesDir)\mlnetmkldeps\$(MlNetMklDepsPackageVersion)\LICENSE.txt" Pack="true" PackagePath=".\NOTICE.txt" /> |
There was a problem hiding this comment.
I don't think we need the license file here since we are not packaging MKL in this nuget. #Resolved
| @@ -754,6 +754,11 @@ private void CheckLabel(RoleMappedData examples, out int weightSetCount) | |||
|
|
|||
| private static unsafe class Native | |||
| { | |||
| static Native() | |||
| { | |||
| var t = ErrorMessage(0); | |||
There was a problem hiding this comment.
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
| @@ -754,6 +754,11 @@ private void CheckLabel(RoleMappedData examples, out int weightSetCount) | |||
|
|
|||
| private static unsafe class Native | |||
| { | |||
| static Native() | |||
| { | |||
| var t = ErrorMessage(0); | |||
There was a problem hiding this comment.
Let's add the reasoning/comment here on why we are doing this. #Resolved
| @@ -18,4 +18,8 @@ endif() | |||
| add_library(SymSgdNative SHARED ${SOURCES} ${RESOURCES}) | |||
| 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}") | |||
There was a problem hiding this comment.
${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
| target_link_libraries(MklProxyNative PUBLIC ${MKL_LIBRARY}) | ||
|
|
||
| if(APPLE) | ||
| set_target_properties(MklProxyNative PROPERTIES INSTALL_RPATH "@loader_path;${MKL_LIB_PATH}") |
There was a problem hiding this comment.
We shouldn't need MKL_LIB_PATH here, right? MklProxyNative will always be in the same directory as MklImports. #Closed
src/Native/build.proj
Outdated
| <ItemGroup> | ||
| <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)MklImports$(NativeLibExtension)" | ||
| RelativePath="Microsoft.ML.HalLearners\runtimes\$(PackageRid)\native" /> | ||
| <NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)MklProxyNative$(NativeLibExtension)" |
There was a problem hiding this comment.
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
src/Native/build.proj
Outdated
| @@ -53,7 +53,7 @@ | |||
| DependsOnTargets="GenerateVersionHeader"> | |||
|
|
|||
| <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 was a problem hiding this comment.
There's no such thing as 'rpath' on Windows, right? We shouldn't need to use/pass it on Windows. #Closed
src/Native/build.sh
Outdated
| --mkllibrpath) | ||
| shift | ||
| __mkllibrpath=$1 | ||
| ;; |
There was a problem hiding this comment.
(nit) tabs at the end of this line #Resolved
| CceFormat = 57 /* Complex conjugate-even */ | ||
| }; | ||
|
|
||
| EXPORT_API(int) DftiSetValue(void *handle, ConfigParam config_param, ...); |
There was a problem hiding this comment.
It would be nice having a comment here explaining why we need to make these "proxy" methods. #Closed
| @@ -18,4 +18,8 @@ endif() | |||
| add_library(SymSgdNative SHARED ${SOURCES} ${RESOURCES}) | |||
| 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}") | |||
There was a problem hiding this comment.
@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
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Content Include="..\common\CommonPackage.props" Pack="true" PackagePath="build\netstandard2.0\$(MSBuildProjectName).props" /> |
There was a problem hiding this comment.
TimeSeries doesn't ship any native assemblies, so it doesn't need this .props file. #Resolved
…to timeseries # Conflicts: # Microsoft.ML.sln
ghost
locked as resolved and limited conversation to collaborators
Port of time series prediction.
fixes #978, #1092 and #1103