issue_comments

@r-beer in case you come back to see this: thank you for taking it so far; your code was helpful to eventually getting this feature in. And we'd of course appreciate any additional contributions.

Either way, it would be good to have a clear list of da_a and da_b and respective expected results. I am a bit confused about what we actually would expect for xarray.DataArray([[1, 2], [np.nan, np.nan]], dims=['x', 'y']).

For xarray.corr(array, array, dim='y'), I think we'd expect an array of the same dimensionality as below, with one point as the correlation of 1.0 (because {1,2} is 100% correlated to {1,2}) and another NaN. Does that make sense?

In [17]: xr.dot(da,da, dims='y') Out[17]: <xarray.DataArray (x: 2)> array([ 5., nan]) Dimensions without coordinates: x

@shoyer good spot, thanks @r-beer we could add that as a test case

Yes, I will add it.

Thinking about test arrays in general: Wouldn't it be good to define some data array fixtures in the conftest.py which can then be used in all different test_function.py tests?

I think we could do more of this, yes. It's a tradeoff between (a) better sharing & performance vs. (b) customization for each function & localization of effects. We have def create_test_data in a few files, before we used pytest, so could convert some of the usages to fixtures. @shoyer and I discussed this briefly with @keewis 's work on tests for units. I think @shoyer is keener on having fixture-like code close to the test functions.

In view of the additional test I started to restructure the test_cov and test_corr to a single test_func. I am now asking myself how much of the data preprocessing shall go to the parametrization and how much remains in the test function...

Any guidelines or suggestions?

I would start with having the code in the function; i.e. don't abstract too early unless we're really confident on the eventual state. When we find ourselves something multiple times, then we can start pulling it out into more abstractions (e.g. fixtures).

@shoyer good spot, thanks

@r-beer we could add that as a test case

@r-beer great list of future PRs!

@shoyer take a glance if you get a second. I'm deliberately pushing this through even though https://github.com/pydata/xarray/pull/3550/files#diff-293280a1ed0154c9fb35b842f69e7e33R1112 isn't resolved, since I think these are important features which have dragged for years, and we can iterate on the performance (and @r-beer has done a great job on his first PR!)

Oh, ok. I thought I have to create xarray-docs to update whats-new.rst and api.rst?

You can just edit the files directly! No need to build all the docs. It should be much simpler than the work you've already done!

(lmk if the docs were unclear and we can update)

@r-beer let's go with @shoyer 's suggestion to change this from a method to a function xr.cov & xr.corr. Is that OK?

I am now looking into the generation of the documentation.

What do you mean by this? You shouldn't have to generate anything locally?

@r-beer your current test case looks fine, no stress on the fixtures!

One question we started discussing on the old PR which we should resolve ASAP given @r-beer 's work: do we want xr.cov(da1, da2) or da1.cov(da2)?

CC @fujiisoup @shoyer

Great start!

Thanks for the review @keewis

I think it's fine to leave Dataset for another PR; it'd be great to get this in, and given it's your first PR (welcome!) let's not bloat it

Test looks good. Let's make those adjustments you put in the TODO and add one for the other function? We could also add some variants which test along different dimensions / all NaNs / other special cases.