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!)

where should these functions be placed?

Sorry, I misunderstood this question. I thought you were asking about documentation but I see that you're asking about which .py file to stick these functions in. Let's do computation just like xr.dot.

PS: It looks like xr.dot is only present in the "API reference" and not in https://xarray.pydata.org/en/stable/computation.html. We should add dot, cov and corr to computation.rst in a future PR.

Computation? Where is xr.dot documented?

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?

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)?

In my opinion the function form is clearer, because cor and corr are symmetric in both arguments.

@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

I'm not sure about the examples in the examples folder, I think those are real-world examples?

What I meant was examples like those in DataArray.integrate (source) that simply demonstrate how to use that function.

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.

since the APIs of DataArray and Dataset are usually kept in sync as much as possible, it might be good to add the new methods to Dataset, too. This doesn't have to be in this PR, though, and I'd wait for confirmation first.

for the new commit, yes. The older commits could be rewritten if you don't like the other email address being public (I can guide you through the process if you want) but since PRs get squashed during the merge you don't have to.

yes, whats-new.rst and api.rst will have to eventually be updated before the merge. Also, examples would be really good.

There seems to be a problem with your email address (github can't match it to your account). Does it match the one you used for signing up on github?