Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix documentation of ewma_by_time #74

Open
azmyrajab opened this issue Apr 21, 2024 · 2 comments
Open

fix documentation of ewma_by_time #74

azmyrajab opened this issue Apr 21, 2024 · 2 comments

Comments

@azmyrajab
Copy link

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
Copy link
Collaborator

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
Copy link
Author

azmyrajab commented Apr 22, 2024

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};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants