-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cubed xarray tests #4
base: main
Are you sure you want to change the base?
Changes from all commits
1532ec8
9be6e3c
df26fc7
72fd4bf
7f2654b
376181c
4610b30
77f08c8
6d77f30
3b16694
f97633d
3e66118
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,3 +10,5 @@ dependencies: | |
- hypothesis | ||
- xarray | ||
- numpy | ||
- cubed | ||
- cubed-xarray |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
from contextlib import AbstractContextManager as ContextManager | ||
from contextlib import nullcontext | ||
|
||
import cubed | ||
import cubed.random | ||
import hypothesis.strategies as st | ||
import numpy as np | ||
import numpy.testing as npt | ||
import pytest | ||
from hypothesis import note | ||
|
||
from xarray_array_testing.base import DuckArrayTestMixin | ||
from xarray_array_testing.creation import CreationTests | ||
from xarray_array_testing.reduction import ReductionTests | ||
|
||
|
||
def cubed_random_array(shape: tuple[int], dtype: np.dtype) -> cubed.Array: | ||
""" | ||
Generates a random cubed array | ||
|
||
Supports integer and float dtypes. | ||
""" | ||
# TODO hypothesis doesn't like us using random inside strategies | ||
rng = np.random.default_rng() | ||
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a Hypothesis-provided seed? I'd also be happy to accept a PR to generate Numpy prng instances 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably try using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
|
||
if np.issubdtype(dtype, np.integer): | ||
arr = rng.integers(low=0, high=+3, size=shape, dtype=dtype) | ||
return cubed.from_array(arr) | ||
else: | ||
# TODO generate general chunking pattern | ||
ca = cubed.random.random(size=shape, chunks=shape) | ||
return cubed.array_api.astype(ca, dtype) | ||
|
||
|
||
def random_cubed_arrays_fn( | ||
*, | ||
shape: tuple[int, ...], | ||
dtype: np.dtype, | ||
) -> st.SearchStrategy[cubed.Array]: | ||
return st.builds(cubed_random_array, shape=st.just(shape), dtype=st.just(dtype)) | ||
|
||
|
||
class CubedTestMixin(DuckArrayTestMixin): | ||
@property | ||
def xp(self) -> type[cubed.array_api]: | ||
return cubed.array_api | ||
|
||
@property | ||
def array_type(self) -> type[cubed.Array]: | ||
return cubed.Array | ||
|
||
@staticmethod | ||
def array_strategy_fn(*, shape, dtype) -> st.SearchStrategy[cubed.Array]: | ||
return random_cubed_arrays_fn(shape=shape, dtype=dtype) | ||
|
||
@staticmethod | ||
def assert_equal(a: cubed.Array, b: cubed.Array): | ||
npt.assert_equal(a.compute(), b.compute()) | ||
|
||
|
||
class TestCreationCubed(CreationTests, CubedTestMixin): | ||
pass | ||
|
||
|
||
class TestReductionCubed(ReductionTests, CubedTestMixin): | ||
@staticmethod | ||
def expected_errors(op, **parameters) -> ContextManager: | ||
var = parameters.get("variable") | ||
|
||
xp = cubed.array_api | ||
|
||
note(f"op = {op}") | ||
note(f"dtype = {var.dtype}") | ||
note(f"is_integer = {cubed.array_api.isdtype(var.dtype, 'integral')}") | ||
|
||
if op == "mean" and xp.isdtype( | ||
var.dtype, ("integral", "complex floating", np.dtype("float16")) | ||
): | ||
return pytest.raises( | ||
TypeError, match="Only real floating-point dtypes are allowed in mean" | ||
) | ||
elif xp.isdtype(var.dtype, np.dtype("float16")): | ||
return pytest.raises( | ||
TypeError, match="Only numeric dtypes are allowed in isnan" | ||
) | ||
elif op in {"var", "std"}: | ||
pytest.skip(reason=f"cubed does not implement {op} yet") | ||
else: | ||
return nullcontext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay thanks. Sounds like perhaps we should disable this warning globally (if possible) and just report if it actually causes problems.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not only that, unfortunately: if you want to check how
dask
andcupy
work together, for example,cupy-xarray
would have to create both the suite forcupy
and thedask+cupy
one.The other option we'd have is to generate a single test class within a function:
which would avoid the reuse of a single
given
, but this quickly becomes tricky to read because of the deeply nested structure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to work around this in #7 by delaying the application of
given
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).