pola-rs / polars-xdt

Polars plugin offering eXtra stuff for DateTimes
https://marcogorelli.github.io/polars-xdt-docs/
MIT License
187 stars 8 forks source link

fix documentation of ewma_by_time #74

Open azmyrajab opened 6 months ago

azmyrajab commented 6 months ago

Thanks for the nice package. A minor ask -

I was trying to follow the maths w/ ewma_by_time and noticed what I think is a minor typo:

In docstring:

        .. math::

            y_0 &= x_0

            \alpha_i &= \exp(-\lambda(t_i - t_{i-1}))

            y_i &= \alpha_i x_i + (1 - \alpha_i) y_{i-1}; \quad i > 0

In actual code, and what I think is correct:

let delta_time = time - prev_time;
// equivalent to:
// alpha = exp(-delta_time*ln(2) / half_life)
let alpha = (0.5_f64).powf(delta_time as f64 / half_life as f64);
let result = (1. - alpha) * value + alpha * prev_result;

Notice alpha hits the previous result (i.e. should be y_{i-1}) and 1 - alpha hits latest data (x_i).

If you agree, it might make sense to do the same w/ the upstreamed polars ewm_mean_by.

Anyways, thanks for the nice work on this feature.

MarcoGorelli commented 6 months ago

I think this is correct, the variable name here is just backwards. If you check the code in polars itself, I think there it's the right way round?

azmyrajab commented 6 months ago

Hi Marco,

yes the code is definitely correct in both polars and extension package.

let result = (1. - alpha) * value + alpha * prev_result;

but in the docstring (regardless of order), alpha i is being multiplied by x_i (new data) and no the previous state: y_i &= \alpha_i x_i + (1 - \alpha_i) y_{i-1};

I think the docstrings should be: y_i &= \alpha_i y_{i-1} + (1 - \alpha_i) x_{i};