Cubed xarray tests

[TomNicholas]

No description provided.

[dcherian]

If its useful, I wrote a chunks strategy here:
https://github.com/xarray-contrib/flox/blob/edc13445a098ee95f074a4bd92bde1c48694804c/tests/strategies.py#L115-L127

though it does generate non-uniform chunks

[TomNicholas]

I wrote a chunks strategy here:

@dcherian I also wrote one at dask/dask#9374 😆

It will be useful eventually, but right now we're trying to get the testing framework in place that the chunk strategy would plug into. The chunk-generating strategy would be called by the cubed_random_array strategy.

[TomNicholas]

@Zac-HD the fact we had to add this seems to indicate a possibly-serious misuse of hypothesis, but in some way that @keewis and I struggled to properly understand from looking at the docs.

https://hypothesis.readthedocs.io/en/latest/settings.html#hypothesis.HealthCheck.differing_executors

[Zac-HD]

see HypothesisWorks/hypothesis#3446 for the motivating cases; if you inherit an @given() test onto multiple child classes with different behavior you can get some pretty weird behaviors.

If you don't observe anything like that, it's probably okay albeit fragile.

[TomNicholas]

In the very limited running we did today, we didn't observe anything unexpected after we disabled the health check.

But is there some other pattern we should be using here?

[Zac-HD]

I don't have any concrete suggestions; inheritance for code-sharing is both useful in this kind of situation, and also prone to sharing slightly more state than we want it to. A design that doesn't use inheritance would be safer but I'm not sure it's worth the trouble.

[TomNicholas]

Okay thanks. Sounds like perhaps we should disable this warning globally (if possible) and just report if it actually causes problems.

if you inherit an @given() test onto multiple child classes with different behavior

We are not actually ever going to be inheriting one given test onto multiple child classes, only onto one child class (per downstream package). So maybe that makes it okay?

...actually the one exception to that statement would be in Xarray itself, where we would inherit once to test wrapping numpy, once to test wrapping dask etc. But we could probably still set up our CI to ensure that only one child test class (suite of children really) gets run per CI job.

[Zac-HD]

Should be fine, iirc it's only an issue if you're replaying test cases from the database and the underlying sequence of choices is different and you hit a particular unlucky situation.

[keewis]

actually the one exception to that statement would be in Xarray itself

not only that, unfortunately: if you want to check how dask and cupy work together, for example, cupy-xarray would have to create both the suite for cupy and the dask+cupy one.

The other option we'd have is to generate a single test class within a function:

def generate_tests(name, array_strategy, array_type, xp):
    @rename_class(f"Test{name.title().replace('_', '')}Array")
    class TestDuckArray:
        class TestCreation:
            array_strategy_fn = array_strategy
            ...

    return TestDuckArray

TestNumpyArray = generate_tests("numpy", create_numpy_array, np.ndarray, np)

which would avoid the reuse of a single given, but this quickly becomes tricky to read because of the deeply nested structure.

[keewis]

I've tried to work around this in #7 by delaying the application of given.

[TomNicholas]

Whilst #7 is clever I think my conclusion from @Zac-HD 's comments above is that the extra complexity is unnecessary - it's fine to just suppress the warning and we only need to revisit this issue if we ever actually observe weird behaviour (i.e. YAGNI).

[Zac-HD]

Consider using a Hypothesis-provided seed? I'd also be happy to accept a PR to generate Numpy prng instances 🙂

[keewis]

we should probably try using xps.arrays() instead (though I guess that only works for array API compliant duck arrays)

[Zac-HD]

The other argument against is that sometimes you just want a faster PRNG for the elements; the distribution is a bit less likely to find bugs but setting elements individually is a lot slower (even though we do a sparse subset)

[keewis]

there's two distinct failures here:

1. mean does not support complex dtypes (complex64, complex128) which are both part of the array API. I've marked that as an expected failure for now.
2. The reductions somehow run eagerly instead of lazily. (Even assuming that eager computation is okay, the reductions should return numpy.ndarray, but on numpy>=2.1 they return numpy scalars; this is a known bug upstream in xarray)

[keewis]

@tomwhite, I just tried to get xarray to allow you to define __array_function__ on cubed. This was surprisingly simple, I just had to:

1. switch the order of the checks for __array_namespace__ and __array_function__ (as described in Missing array-api support for some stats functions? pydata/xarray#8834 (comment))
2. re-apply the PR you had reverted on cubed
3. define nanprod on cubed
4. remove out=out in xarray's dispatch to nanprod (which we should do anyways, considering that that's the only time xarray is actually passing through out).

with those four changes, xarray's test suite still passes in my environment, and the tests here don't fail because cubed has been eagerly computed. However, there's some floating point issues we still have to resolve (mostly floating-point errors for float32 / complex64).

[tomwhite]

@keewis awesome!