BUG: Use stable algorithm for _nanvar.
State: closed

closes #10242

This PR replaces the sum-of-squares algorithm used to compute the variance by a more stable algorithm. The algorithm here is essentially the same as used in numpy 1.8 and up, and I've added a TODO to replace the implementation with a direct call to numpy when that version is the default.

Somewhat counter to the discussion in #10242, I chose not to go with the Welford algorithm, for two reasons: numpy, and the fact that `_nanvar` needs to be able to deal with arrays of different shape, which is tricky to get right in Cython.


Labels:
name: Bug
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
@shoyer or @jreback Would either of you mind giving this a quick review? Thanks!

So the plan is to replace this with numpy's version once we bump to that to 1.8? Could you open up a separate issue as to reminded us to do that?

we won't be eliminating 1.7 for quite some time FYI 
so this would have to be a conditional (and could even be out in now with the conditional on the version)

This looks pretty reasonable to me.

Thanks all for the extensive comments!

@jreback I made a first pass at addressing your comments. Regarding the relative lack of tests, I assumed that the existing tests in `test_nanops` would be sufficient to check the behavior of the function in the face of NaNs, different ddofs, etc. However, I noticed that the test infrastructure in `test_nanops` was missing a `raise`, causing some test errors to be swallowed.  I can make that commit into a separate PR if necessary.

@jvkersch I would like to see an explicty test that checks against a hard value (with an w/o nans), just to lock this down (rather than computing agains `np.var` which is what we do IIRC). That is generally fine, but since you are addressing this, can't hurt.

Just a quick note: I haven't forgotten about this, but I'm currently lacking the time to finish this off. What's hindering me a bit is the lack of uniformity in the api for the nanops: some cast their arguments to float64 right away, some only do that for int dtypes, ... there's the implicit assumption that the nanop will raise `TypeError` for non-numeric input, etc. I think it might be worth fleshing out this api in a future PR. If I can't get things to work in a satisfactorily manner I'll just go for the minimally viable implementation here, and work on that instead, before revisiting.

@jreback Would you be able to give this another review? Since last time, I took care of your earlier comments (explicit tests, folding `_nanvar` into `nanvar`, using a wide accumulator), but some unexpected side-effects plagued me a little:
1. Since `nanstd` called `_nanvar` directly, having it call `nanvar` instead meant fixing up some tests.
2. Numpy generates a RuntimeWarning whenever the number of dofs is not strictly positive. To avoid being swamped by those warnings, I turned off RuntimeWarnings when calling Numpy's `var` or `nanvar`.
3. Some tests for `numeric_only` called `nanvar` with non-numeric data and listened for a `TypeError`. This happened to work with the earlier implementation, but in some cases the new implementation raises a `ValueError` instead. Implementation-wise this works fine, too, but one test needed to be made a little more flexible.

Thanks!

@jreback I've added some tests for `nanstd` (though they are not as extensive as the `nanvar` tests). I couldn't get things to work reliably with `tm.assert_array_equal` (because of the fixed precision) so I'm hoping this can stay as-is (and I wouldn't mind revisiting this after #10788 has been addressed). Does this look good to you?

looks good. pls rebase/squash. ping when green.

@jreback Done!

merged via https://github.com/pydata/pandas/commit/2d0d58c04114b634072e9425824b34cda3aeaef5

@jvkersch awesome job!

thanks!

@jreback Thanks for the swift and thorough review!