Purpose of testing np_deterministic and ufunc

[aaronspring]

There's probably a bigger question about the purpose of the tests etc.

Originally posted by @raybellwaves in #247 (comment)

I would say that testing that ufunc (incl or excl dask) does the same as the np functions, is not required for xs. We don’t interfere there, do we? How should these tests break anyways? I am concerned about alleged security when these tests are present.

[aaronspring]

In my opinion we would need a big PR checking that each standard keyword, I.e. (dim) skipna, weighted works as expected. I feel that we missed some combinations in the past. @raybellwaves just caught some. Are those all of them?

This test can be done in either np_deterministic or deterministic.py, probably the later better to test for chunked and has nans for has zeros also. Testing both np_det and det.py seems redundant to be as xr.applyufunc just calls np_det function and carries xr properties.

list of testing inputs possible:

• dataset, dataarray
• chunked, eager
• 1d, 3d (both needed?)
• Has random nans - Has random zeros (combine or not)
• Has fixed nans, zeros (land land/sea mask)

[raybellwaves]

slightly related comment at the top here (#247 (comment))

[raybellwaves]

I'm in favor of well named test files which are clearly defined with and built with hierarchy.

For example currently we have:

• https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/test_metric_results_accurate.py
  which checks the math of the metric (i.e. the implementation in np_*.py)
• https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/test_deterministic.py which checks the output of the ufunc matches the np array (see @aaronspring's comment on top. Also interesting to note that this file is now very different from the first version 93de818#diff-7bd4c759666838b7ef538559ce91405eb9580076e3ac784d6a81519113bad375). In this script we also have test_*_metrics_daskda_same_npda. IMO these should come out of this file and be in a separate file which only tests dask objects i.e. we only move this phase once firstly, we know the math of the metric is right and secondly, the unfunc is working with xr.DataArray's with data in memory (np array).

Aaron also mentioned the use of pytest-lazy-fixture (#247 (comment)). This seem very useful and is used in climpred e.g. https://github.com/pangeo-data/climpred/blob/fdec3af0aabac42de1787aecc0fcbef64a4800f7/climpred/tests/test_bootstrap.py#L227. It has the benefit of writing one test but parameterizing variables. as example is

@pytest.mark.parametrize(
    "a, b",
    [
        (
            pytest.lazy_fixture("a_dask"),
            pytest.lazy_fixture("b_dask"),
        ),
        (
            pytest.lazy_fixture("a_dask_rand_nan"),
            pytest.lazy_fixture("b_dask_rand_nan"),
        ),
        (
            pytest.lazy_fixture("a_dask_with_zeros"),
            pytest.lazy_fixture("b_dask_with_zeros"),
        ),
    ],
)
def test_dask(a, b, ...)

Which saves writing the same test three times (if the code is the same)

def test_dask(a_dask, b_dask, ...)
def test_dask_rand_nan(a_dask_rand_nan, b_dask_rand_nan, ...)
def test_dask_with_zeros(a_dask_with_zeros, b_dask_with_zeros, ...)

But it's ideal not to mix and match these approaches within the same script. For example, there could be a small simple dask test file where the above code is applicable

I think a good starting place is adding the possible inputs to https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/conftest.py which Aaron recently added.

I'm also happy for this issue convo to be like a scratch pad to discuss ideas/breaking down PRs etc.