BUG: normalize rolling_window() weights, remove scikits-timeseries dependency for testing
State: closed

This closes #7618 and removes the dependency on scikits-timeseries for testing rolling_window functions. Scikits-timeseries has not been maintained since 2009, and I had difficulty simply installing it on my system. Instead of using scikits-timeseries to calculate the various rolling moments on randomly generated data, the tests use pre-calculated data.


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
nice!

cc @seth-p can you have a look

cc @AllenDowney pls have a look as well

@stahlous also pls remove `scikits.timeseries` from `pandas/utils/print_versions`.

I removed `scikits.timeseries` from `pandas/utils/print_versions` (please forgive the stupid commit message saying I removed "scikits.learn - was using that package recently).

About this PR, I wonder if there's any concern about this breaking the API, even though this does fix a bug. It is conceivable that someone has been using it in its current manifestation and is expecting it to provide the current errant behavior.

@stahlous pls put an example in v0.15.0.txt in the API section (you can use a code-block to show what it would do previously) and an ipython block to show current code (see what cc @seth-p did for example a bunch of fixes for ewma).

This looks good to me.  Thanks!

@stahlous can you squash?

@jreback, I will try to review later today.

Just looking at the tests, I would think there should be a test that applies all the rolling window types to a non-zero constant Series (and DataFrame), with `center` equal to both `True` and `False`, checking that the result is the same as the input (up to some `NaN` values at the edges, as expected based on the `window` and `center`). This seems like the best/simplest test of overall scaling/normalization of the weights. Apologies if such a test exists and I missed it.

The non-zero constants is a great idea - easy to calculate and simple to construct on the fly. I'll add those tests on top of the pre-calculated results for randomly selected data that I already implemented. 

I'll try to implement all of the suggestions so far this evening.

More generally, you can similarly test that for linear data, when window is odd and center=True (I think), the output should equal the input (save perhaps at the edges). This assumes that the weights are all symmetric, which I haven't checked but assume is true.

> On Sep 12, 2014, at 5:03 PM, stahlous notifications@github.com wrote:
> 
> The non-zero constants is a great idea - easy to calculate and simple to construct on the fly. I'll add those tests on top of the pre-calculated results for randomly selected data that I already implemented.
> 
> I'll try to implement all of the suggestions so far this evening.
> 
> —
> Reply to this email directly or view it on GitHub.

I squashed the commits, added the API documentation, and added more tests using linear series.

What is the purpose of `avg_wgt` in `algos.roll_window()`? So far as I can tell it is not documented and is never explicitly set, and so defaults to `False`. It seems like this might accomplish the same thing as your fix. At any rate, if it is never used and is redundant, perhaps best to delete it (and all the code under `if avg_wgt:`)? Or else perhaps change `rolling_window()` to set it when calling `algos.roll_window()`?

On a related note, is it clear what the expected normalization is when requesting a sum rather than a mean from `rolling_window()` (i.e. `mean=False`)? You're normalizing things so that the sum of the weights is equal to the window length (when `win_type` is specified; if `window` is a list/tuple/ndarray, the specified values are left unmolested). That seems perfectly reasonable to me, though should probably be made explicit.

Well, how about that...

```
import scipy.signal as sig
s = np.array(np.random.randn(20)+10)
window = sig.get_window('triang', 5).astype(float)
m1 = pd.algos.roll_window(input=s, weights=window, minp=5, avg=True, avg_wgt=False)
m2 = pd.algos.roll_window(input=s, weights=window, minp=5, avg=True, avg_wgt=True)
plt.plot(s, 'b', m1, 'g', m2, 'r')
```

![image](https://cloud.githubusercontent.com/assets/7545449/4278138/3abebf32-3d15-11e4-9a89-b31bd4d64765.png)

I'm not sure yet if the two methods are materially different or not. It may be someone has been smarter about this than I.

Looks like it does the same thing as your fix, no?

Not being particularly Cython-savvy, can someone explain to me what these, presumably Boolean, expressions are supposed to accomplish?

```
        if val_win != val_win:

        if val_in == val_in:
```

It does look like it's the same as my fix. However, if they're materially the same then my fix should presumably be faster since it's not calculating weights on the fly. However, I'm wondering if there are missing data concerns or something along those lines that I haven't considered.

a nan != nan so easy way to test if a number is a nan

As suspected, the logic `roll_window()` handles missing data properly where as my naive method produces nonsense answers when there's missing data.

```
import scipy.signal as sig
n=7
win = 5
minp = 3
s = np.array(np.random.randn(n)+10)
window = sig.get_window('triang', win).astype(float)
m1 = pd.algos.roll_window(input=s, weights=window, minp=minp, avg=True, avg_wgt=False)
m2 = pd.algos.roll_window(input=s, weights=window, minp=minp, avg=True, avg_wgt=True)
m3 = pd.rolling_window(s, window=win, win_type='triang', min_periods=minp)
plt.plot(s, 'b', m1, 'g', m2, 'r', m3, 'y')
ticks = plt.xticks(range(n))
```

![image](https://cloud.githubusercontent.com/assets/7545449/4280033/f1851ab0-3d27-11e4-8b35-c266a3289bd7.png)

My fix actually causes the calculated mean to be actually higher than the input data where there's missing data.

So, the proper fix is to update `rolling_window` to invoke `roll_window` with `avg_wt=True`. The question is, do we want to update the `rolling_window` API to allow turning the normalization off? Someone must have thought that was an important possibility at some point to have written `roll_window` to allow both cases.

Well, how is "turning normalization off" different from specifying `mean=False` (i.e. want sum rather than mean)?

Seems to me that `algos.roll_window()` has too many degrees of freedom:
(a) if `avg_wgt == True`: normalize total weight used to 1.0; else
(b) if `avg == True`: normalize by dividing by number of weights used; else
(c) use weights as provided without any normalization.

Observations/comments:
1. I believe the code will crash if `avg == False` and `avg_wgt == True`, as `total_wgt` will be accessed but not allocated. Other than that, If `avg_wgt == True`, then `avg` is ignored. So `avg_wgt == True` better imply/set ``avg == True`. Seems awkward.
2. Is there a use case for (b) (as distinct from (a))? None comes to mind.

My thought would be:
(i) In `algos.roll_window()`, delete `avg` and have `avg_wgt` default to `True`.
(ii) In `rolling_window()`, call `algos.roll_window()` with `avg_wgt=mean` (as opposed to the current `avg=mean`).
(iii) That still leaves open the choice of normalization for weights from `sig.get_window(win_type, window)` -- which matters only if `mean=False`. I think normalizing them to sum to 1.0 makes the most sense, though also possible to normalize so that the maximum value is 1.0.

I think this would be backwards compatible (except for the case discussed in (iii), which needs fixing anyway), but should confirm...

I removed `avg_wgt` from `roll_window()`. Now when `avg=True` (the default condition) you get normalized weighted means. When `avg=False` you get weighted sums. This is the same as the current behavior except that means are normalized and there's no way to get non-normalized means (and I can't think of a reason why you would want to).

I also added an extra test that includes a missing data point and `min_periods` < `window`.

Looks good to me.

Are there tests (with `avg` both `True` and `False`) for user-specified weights, i.e. when `window` is an array of weights?

When specifying a `win_type` and `avg=False`, is the scaling well-defined? It's not obvious to me.

I think the scaling when `avg=False` is correct. I was considering that we might want to normalize the weights by the (window length / sum of window weights) factor that we had previously considered implementing, but that would result in members of the window whose weighting is greater than 1, which is strange in the context of a sum.

I'll add a test for user-specified weights.

What do you mean by "correct"? Seems to me you're at the mercy of scipy. Looking at http://docs.scipy.org/doc/scipy-0.14.0/reference/generated/scipy.signal.get_window.html, they don't seem to say anything about the scaling, though from the three examples there it looks like the scaling is so that largest weight is `1`.

@stahlous  / @seth-p 

so no rescaling occurs? (or rather, are their certain window types where it SHOULD occur, or already does?)

It looks to me like `scipy.signal.getwindow()` always scales the weights so that the maximal weight is `1.0` (though for `flattop` it seems to be `1.0002` - don't ask me why). That seems reasonable to me. But perhaps worth mentioning in the docs.

This is starting to get a little philosophical, but I think the problem is deciding what the user should expect. I think in the case of a rolling mean, it is the correct thing to assume that the user expects the weighted mean to fit within the distribution of the points used to calculate it. The behavior reported in #7618 is certainly non-intuitive. If I pass in a series of [10, 10, 10] I would expect the weighted mean to be 10 and not ~6. The way the function is currently written in this PR seems to me the only proper way for a rolling mean to behave.

However, when it comes to a rolling sum, the expected behavior is a little more ambiguous. Imagine I pass in the weights [2, 2, 2] and a series of [1, 1, 1].  Would the user expect that the result would be [nan, nan, 6] or [nan, nan, 3]? The function as written in the PR would return [nan, nan, 6]. I don't have a great argument for either way being the proper result.  I don't think that the same weight normalization used in the rolling mean calculation is appropriate. If anything the weights for a rolling sum should be scaled as `window = window / window.max()`.

By the way, is there much practical use for rolling weighted sums? A google search for "weighted rolling sum", "rolling weighted sum", "moving weighted sum" and "weighted moving sum" returns on the order of a few dozen hits total. Maybe the rolling sum aspect of this function should be dumped altogether or moved to a different function to avoid ambiguity.

how about adding a `normalize=True` or `normalize_weights=True` kw ? THen its not ambiguous, and can do separate behavior for `rolling_mean` (normalize=True) vs `rolling_sum` (normalize=False)