-
Notifications
You must be signed in to change notification settings - Fork 21
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
Test chunking (including Hypothesis tests) #57
Conversation
One of the CI runs (ubuntu-latest, 3.9) has 10 |
Do you think the failures are implementation dependent? In other words, should I merge this branch with #49 and see if the tests fare any better? Or do you think there is a problem with the tests themselves? |
This branch builds atop #49 so if you merge them you will only end up with exactly the same code that's here. Even locally I don't get a consistent number of failures - I just ran the whole suite 3 times and got 17, then 19, then 18 failures. 😕 What is consistent is that every parametrization of the the I don't know what could be causing the non-deterministic behaviour apart from the dask |
I just tried that in #58 (messed up a rebase before realising I actually needed to cherry-pick), but the tests still fail. Similar test behaviour - the same tests fail, though now a lot of them fail with xhistogram/test/test_chunking.py:156: in test_all_chunking_patterns_dd_hist
h = histogram(*[da for name, da in ds.data_vars.items()], bins=bins)
xhistogram/xarray.py:163: in histogram
h_data, bins = _histogram(
xhistogram/core.py:339: in histogram
bin_counts = _histogram_2d_vectorized(
xhistogram/core.py:163: in _histogram_2d_vectorized
bin_indices = ravel_multi_index(each_bin_indices, hist_shapes)
xhistogram/duck_array_ops.py:24: in f
return getattr(module, name)(*args, **kwargs)
<__array_function__ internals>:5: in ravel_multi_index
???
../../../../miniconda3/envs/py38-mamba/lib/python3.8/site-packages/dask/array/core.py:1551: in __array_function__
return da_func(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
multi_index = [dask.array<digitize, shape=(1, 72), dtype=int64, chunksize=(1, 1), chunktype=numpy.ndarray>, dask.array<digitize, sha... chunktype=numpy.ndarray>, dask.array<digitize, shape=(1, 72), dtype=int64, chunksize=(1, 1), chunktype=numpy.ndarray>], dims = [9, 10, 11, 12], mode = 'raise', order = 'C'
@wraps(np.ravel_multi_index)
def ravel_multi_index(multi_index, dims, mode="raise", order="C"):
> return multi_index.map_blocks(
_ravel_multi_index_kernel,
dtype=np.intp,
chunks=(multi_index.shape[-1],),
drop_axis=0,
func_kwargs=dict(dims=dims, mode=mode, order=order),
)
E AttributeError: 'list' object has no attribute 'map_blocks'
../../../../miniconda3/envs/py38-mamba/lib/python3.8/site-packages/dask/array/routines.py:1763: AttributeError |
That error is #27 (comment) |
I've opened a dask issue to ask about the
Hmm - I guess I could pin my local environment to |
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 just commented over in the upstream dask
issue. If you pass align_arrays=False
to the blockwise
call here, that will avoid the PerformanceWarning
being raised (though there are still other test failures)
Thanks @jrbourbeau , that silences the warning, but unfortunately doesn't fix the failures, and the failures are still inconsistent! 😭 |
I also see flaky tests when trying this PR out locally. FWIW the Interestingly, the failure for this particular test has to do with the Full traceback:
|
See dask/dask#7711 (comment) for more, but I don't think From a quick glance at the failures, it seems like there are generally 2 types of errors:
I haven't looked carefully at these tests yet, but I can try to take a closer look soon. One thing I noticed is that: dims = [random.choice(string.ascii_lowercase) for ax in shape] does allow for the potential of repeated dimension names in the same array. |
Thanks both. This is very helpful.
Makes sense - I'll undo that now. Looks like my
Good point! I've pushed a commit to stop that happening, and everything seems to pass locally now! 🍾 |
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
===========================================
+ Coverage 81.81% 97.18% +15.37%
===========================================
Files 3 2 -1
Lines 242 249 +7
Branches 68 71 +3
===========================================
+ Hits 198 242 +44
+ Misses 37 5 -32
+ Partials 7 2 -5
Continue to review full report at Codecov.
|
Coverage |
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.
Nice! It looks like there are also some linting errors. Could you run the pre-commit
hooks and commit the changes so CI passes?
Yep, I've just fixed them. (flake8 didn't like my fixtures though so I did just have to stick a |
I don't actually think the tests are complete yet though - there should also be tests targeting dask arrays of weights and bins. |
Weights yes. Bins no. I think we want to always require bins to be in-memory. |
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.
Am I missing something, or are the Hypothesis tests gone now?
Co-authored-by: James Bourbeau <[email protected]>
@gjoseph92 I moved them to another file to avoid a linting error with the hypothesis import, but forgot to git add that file before committing at the end of the day yesterday! Thanks everyone for their comments - I think I've addressed them all. I've also turned the fixtures into normal functions, and finally I added a test for chunked weights. One question is whether it would be a good idea to have a test for input arrays with unaligned chunks? |
@TomNicholas I definitely think you should test with unaligned chunks, in both the inputs and the weights. |
Co-authored-by: James Bourbeau <[email protected]>
Co-authored-by: James Bourbeau <[email protected]>
|
||
# TODO mark as slow? | ||
@pytest.mark.parametrize("n_vars", [1, 2, 3, 4]) | ||
@given(chunk_shapes(n_dim=2, max_arr_len=7)) |
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.
Might be nice to also test dims=
and weights=
with Hypothesis. It can be nice to throw all the possible axes of variation into a Hypothesis test as an easy way to check all possible cases, without having to write as many individual tests.
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.
What exactly do you mean? If I make the test_all_chunking_patterns_dd_hist
accept a dims
(or reduce_axes
) argument then I also need a np.histogramdd
function that can handle that generality. Is there a quick way to achieve that in the test? Possibly with np.apply_over_axes
?
For the weights then I guess I could pass weights and allow the data and weights to have different chunking patterns - is that what you meant?
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.
Yeah, I suppose it's trickier to test that, since you'd need something to do N-D histograms (xhistogram) to verify the results.
I suppose you could just compare against histogram
of the computed (NumPy) arrays, and make it purely a test of the dask functionality. If we're confident the NumPy code paths are well-tested, that seems reasonable to me.
But it was just a thought; I think the tests here are already quite good, so fine to leave it as-is too.
Co-authored-by: Gabe Joseph <[email protected]>
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.
Thanks for doing this @TomNicholas. Looks good to me. It's great that these test include testing of multiple dask arguments, which was previously untested and would have caught #48.
I see that this PR is to merge into xgcm:refactor-histogram-map-blocks
which was merged with master
in #49. What's the right way to merge this PR into master
? I'm a bit of a gitwit.
I'm stealing that haha
Good question - apparently you used to have to make a new local branch and push that as a new PR, but now github allows me (or you probably as a maintainer) to edit the target branch directly. |
This builds on #49 by adding a pretty comprehensive set of tests of different chunking arrangements.
There are some normal tests, and some tests that use the Hypothesis library to try out all sorts of different chunk shapes (inspired by @rabernat 's similar test in the rechunker library).
There are some failures, but I think that they are because sometimes dask decides it knows better than me and changes the chunks:
I'm not quite sure how that causes those tests to fail though - I'm not even sure that behaviour is deterministic.
How do I turn this feature off @jrbourbeau @gjoseph92 ? Or alternatively how do I debug what happened tp cause those tests to fail?