Add cumulative moving average function #4555
Conversation
src/shogun/mathematics/Statistics.h
Outdated
| T alpha = (T)(1.0) / n; | ||
| T beta = 1 - alpha; | ||
| linalg::add<T>(datum, cma, result, alpha, beta); | ||
| } |
There was a problem hiding this comment.
You should do a round of clang-format at some point to fix the indents!
src/shogun/mathematics/Statistics.h
Outdated
| template<class T> | ||
| static void cumulative_moving_average( | ||
| const SGVector<T>& cma, const SGVector<T>& datum, | ||
| int32_t n, SGVector<T>& result) |
There was a problem hiding this comment.
Wouldn't it be better to return the result rather than modify the reference?
There was a problem hiding this comment.
Moving averages imply that something is modified so I think is good to just modify the given vector wih the new datum. Not even sure we need the result parameter. Would just call the method “update_mean” and then have only 3 parameters: the mean, the new vector, and n
|
It seems that C's "complex.h" in both clang and gcc defined macros that interfered with the codebase and caused build failures. Including the linalg namespace in "Statistics.h" revealed this. Luckily C++'s "complex" doesn't have this problem. Lapack is the only library that uses "complex.h" but it has the option to use anything else (in lapacke.h) /* Complex type (single precision) */
#ifndef lapack_complex_float
#include <complex.h>
#define lapack_complex_float float _Complex
#endif
/* Complex type (double precision) */
#ifndef lapack_complex_double
#include <complex.h>
#define lapack_complex_double double _Complex
#endifSo #include <complex>
#define lapack_complex_float std::complex<float>
#define lapack_complex_double std::complex<double>before including "Eigen/Eigen" (which includes lapacke) seems to fix the problem for clang and gcc. |
|
Solution: move this to the linalg code. Sorry I didn’t realise earlier that you defined this in the statistics module. Check out how things like the mean are defined in there |
|
I think it would be better if the main problem is solved, since just including linalg in Statistics cause these problems . In clang for example, <complex.h> defines 'I' for complex numbers and google tests uses I as a template parameter name. Check this issue Reference-LAPACK/lapack#320. |
|
Yeah you are right of course. My term solution wasn’t appropriate ;) And Nevertheless, the averaging functions should live in the linalg module. Which also makes this pr independent of the complex problem. Thanks for spotting that btw :) |
ghost
changed the title
ghost
marked this pull request as ready for review
Moving average is implemented in the form average_n = (1-alpha)average_{n-1} + alphax. This has an additional multiplication compared to average_n = average_{n-1} + alpha*(x-average_n), but it should has less rounding error when alpha is small (not sure).