issue_comments

I have a follow-up PR that simplifies the internal logic a little bit more: https://github.com/pydata/xarray/pull/6025

Thanks @Gijom

Sorry for the unresponsiveness. I confirm that using the asfloat conversion also works for me. Should I close the issue or should I let you do it ?

so it probably makes sense to merge it now and save the other bug fix for a later issue?

It was easy .Should be good to go if tests pass.

Checking in here -- it seems like this PR surfaced an existing bug (incorrect calculation of correlation when using dask), at the same time as it corrected a different bug, which is removing the hard dependency on dask for calculating correlation?

If I'm understanding correctly, this is still a net win (fixing one bug), so it probably makes sense to merge it now and save the other bug fix for a later issue?

Unit Test Results

6 files           6 suites   49m 53s :stopwatch: 16 246 tests 14 512 :heavy_check_mark: 1 734 :zzz: 0 :x: 90 672 runs  82 496 :heavy_check_mark: 8 176 :zzz: 0 :x:

Results for commit 3dde39b1.

:recycle: This comment has been updated with latest results.

Ouch here's the problem: da_a has dtype int if it has no NaNs. this dtype could change but need not change after the masking.

Importantly skipna=False by default for int dtypes (since they can't represent NaNs).

The fix is to specify skipna=True explicitly in the .mean calls.

Alternatively we could stick an .astype(float) in _get_valid_values

oh never mind: I can reproduce with ``` da_a = xr.DataArray([[1, 2], [2, 1]], dims=["x", "time"]) da_b = xr.DataArray([[1, 2], [1, np.nan]], dims=["x", "time"])

dim = None

xr.testing.assert_equal( xr.corr(da_a, da_b, dim=dim), xr.corr(da_a.chunk(), da_b.chunk(), dim=dim) ) xr.testing.assert_equal( xr.corr(da_a, da_b, dim=dim), xr.corr(da_a, da_b.chunk(), dim=dim) ) ```

Thanks @Gijom, I can't reproduce. Can you post the output of xr.show_versions()?

Here's mine

I included the proposed changes, thanks for the help.

However the new test I implemented does not go through and I fail to understand why. Would you be able to check ? The idea of the test is to ensure that lazy computations give the same results than normal ones. I tracked down the problem to line 1377 of computation.py: python demeaned_da_a = da_a - da_a.mean(dim=dim) # <-- this one returns nan upon computation (.compute()) demeaned_da_b = da_b - da_b.mean(dim=dim) # <-- this one returns the correct value although the input is masked with nans in the same way

The values before the mean call are: da_a.compute() = <xarray.DataArray <this-array> (x: 2, time: 2)> array([[ 1., 2.], [ 2., nan]]) da_b.compute() = <xarray.DataArray <this-array> (x: 2, time: 2)> array([[ 1., 2.], [ 1., nan]])

And for the means (dim is None): da_a.mean(dim=dim).compute()=<xarray.DataArray <this-array> ()> array(nan) da_b.mean(dim=dim).compute()=<xarray.DataArray <this-array> ()> array(1.33333333)

For non lazy computations everything seems fine.

I was referring to: https://github.com/pydata/xarray/pull/5731#issuecomment-904403621

Otherwise I am working on the conflict with the main branch. Should be done soon.

Thanks @Gijom can you post the error for those tests please.

@keewis indeed this was the problem. I removed it and corrected one test which did not have the @require_dask decorator.

There is still a test error with the test arrays 5 and 6: the lazy array version do not return the same value than the usual corr. I consider this a different bug and just removed the tests with a TODO in the code.

The last commit pass the tests on my machine and I added the whats-new text to version 0.19.1.

The already existing test function test_corr does not have a @requires_dask decorator. But it did not fail as it should have. Not sure why.

the reason for this is that dask is imported using importorskip at the top of the module: https://github.com/pydata/xarray/blob/a96154a85e330ce5aec6c34e49997833c1ae0ba2/xarray/tests/test_computation.py#L27 Since most tests already have requires_dask I think it should be fine to just remove the importandskip call.

Thanks @Gijom!

he already existing test function test_corr does not have a @requires_dask decorator. But it did not fail as it should have. Not sure why.

I can't tell either. It would be good to track this down.

I am not sure of the whats-new.rst format and I thus did not add it. Should I just add a bullet item on the top of the file ?

Yes just copy an existing entry and modify it.

Concerning the tests I added a test to check that lazy correlations have identical results to non-lazy correlations. But this is not directly related to the bug fix. Indeed there is a need to have a test that checks if the non-lazy correlation works without dask installed. The already existing test function test_corr does not have a @requires_dask decorator. But it did not fail as it should have. Not sure why.

About documenting the changes, I am not sure of the whats-new.rst format and I thus did not add it. Should I just add a bullet item on the top of the file ?