Skip to content

DOC: fix conflicting ewma documentation #4499

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

Merged
merged 1 commit into from
Aug 22, 2013

Conversation

Komnomnomnom
Copy link
Contributor

The information in the docs, in the parameter description and in the body of the docstring is conflicting. This should hopefully make them consistent amongst themselves (and with the code).

@@ -75,7 +75,7 @@

EWMA is sometimes specified using a "span" parameter s, we have have that the
decay parameter \alpha is related to the span as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these stray \alphas really be :math:\alpha``?

@Komnomnomnom
Copy link
Contributor Author

Good point! I've updated it so they're all mathified :-)

@cpcloud
Copy link
Member

cpcloud commented Aug 12, 2013

excellent!

@Komnomnomnom
Copy link
Contributor Author

Just noticed #4321 which should be covered by these fixes too (it only updates the docs).

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

are the docs existing docs just inconsistent with the code (before this change / #4321?)

@Komnomnomnom
Copy link
Contributor Author

Yep the docs are inconsistent right now but how they are fixed depends on whether you define alpha = 1 / (1 + c) or alpha = 1 - (1 / (1+c)) = c / (1 + c), unfortunately the current docstring and docs have a mix of both of these.

I've made these changes with alpha = 1 / (1 + c) in mind as that's what I'm familiar with. Here's the relevant bits from the code (algos.pyx)

neww = 1. / (1. + com)
oldw = 1. - neww   
...
output[i] = oldw * prev + neww * cur

Given the definition for alpha = 1 / (1 + com) implies neww = alpha and oldw = 1 - alpha. Assuming prev = output[i-1] = y_t-1 and cur = x_t means the docs should read

y_t = (1 - \alpha) y_{t-1} + \alpha x_t

as they do with this fix, and the fix in #4321.

As I said you can do it all the other way around by defining alpha = 1 - 1 / (1 + c) but you'd still need to update the docs and docstring to make it all consistent (the docs right now define alpha = 1 / (1 + c) but matching the given formula to the code implies alpha = 1 - (1 / (1 +c))). The docstring is similarly conflicted.

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

@Komnomnomnom ok...but your changes make this consistent (with the def of alpha that you are showing)...so that is fine (you might want to mention that it can be done the other way too...., but not go into detail)

@Komnomnomnom
Copy link
Contributor Author

Ok I added a very short note to the docs.

@jreback
Copy link
Contributor

jreback commented Aug 21, 2013

@Komnomnomnom merge?
@cpcloud ok?

@Komnomnomnom
Copy link
Contributor Author

Yep, go for it. If someone wants to change the wording or anything feel free.

jreback added a commit that referenced this pull request Aug 22, 2013
DOC: fix conflicting ewma documentation
@jreback jreback merged commit b364f91 into pandas-dev:master Aug 22, 2013
@Komnomnomnom Komnomnomnom deleted the moments-doc- branch August 31, 2013 23:57
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

Successfully merging this pull request may close these issues.

3 participants