Removed AlignedArray #1657
Removed AlignedArray #1657
Conversation
Anipik
requested review from
danmoseley,
eerhardt,
yaeldekel and
tannergooding
src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.ML.StandardLearners/FactorizationMachine/FactorizationMachineInterface.cs
Outdated
Show resolved
Hide resolved
src/Native/FactorizationMachineNative/FactorizationMachineCore.cpp
Outdated
Show resolved
Hide resolved
Before
After
// The performance is almost the same(within error range) except for native MatmulTrans // end to end Rff Transform @justinormont provided me with the end to end benchmark
I also checked this end to end Benchmark for my earlier change (where I just changed the implementation of matmul and matmulTrans). in that case both before and after was around 850ms. Just for note that something other than my change has made the test slower |
|
@tannergooding can you please take a look at this ? |
src/Microsoft.ML.TimeSeries/AdaptiveSingularSpectrumSequenceModeler.cs
Outdated
Show resolved
Hide resolved
|
It would be good to fix the spelling of |
This file will be removed after this PR |
|
@tannergooding @eerhardt can you please review this one ? i have addressed the feedback |
| @@ -41,7 +41,7 @@ public TimeSeriesEstimatorTests(ITestOutputHelper output) : base(output) | |||
| { | |||
| } | |||
|
|
|||
| [ConditionalFact(typeof(BaseTestBaseline), nameof(BaseTestBaseline.LessThanNetCore30OrNotNetCore))] // netcore3.0 output differs from Baseline | |||
| [ConditionalFact(typeof(BaseTestBaseline), nameof(BaseTestBaseline.LessThanNetCore30OrNotNetCoreAnd64BitProcess))] // 32bit and netcore3.0 output differs from Baseline | |||
There was a problem hiding this comment.
This change seems unrelated. Why is it necessary?
There was a problem hiding this comment.
Our new algorithm leads to slightly different numbers due to different number of multiplications. (in case of floating points). After the change the x86 baseline numbers are almost same as netcoreapp but different from the original x64
|
@tannergooding can you please review this one ? |
|
@tannergooding I have added the code snippet we talked about in all the implementations |
…d matrix and cpumathaligned entirely. moving Aligned array to FactorAware where it is only being used
This reverts commit 72ec121. A bug was detected that resulted in non-deterministic calculation, since the underlying C++ code was written in a way apparently that required alignment to produce consistent results, so of course just removing the alignment and calling an only slightly modified algorithm compromised determinism, resulting in test failure for RFF in particular.
This reverts commit 72ec121. A bug was detected that resulted in non-deterministic calculation, since the underlying C++ code was written in a way apparently that required alignment to produce consistent results, so of course just removing the alignment and calling an only slightly modified algorithm compromised determinism, resulting in test failure for RFF in particular.
ghost
locked as resolved and limited conversation to collaborators
Fixes #1018
Related to #1096