Fix blockwise concatenate for array with some dimension=1.#6342
Fix blockwise concatenate for array with some dimension=1.#6342TomAugspurger merged 1 commit intodask:masterfrom
Conversation
dask/blockwise.py
Outdated
| assert set(numblocks) == block_names | ||
|
|
||
| dummy_indices = all_indices - set(out_indices) | ||
| dummy_indices = sorted(all_indices - set(out_indices)) |
There was a problem hiding this comment.
This is to make the internal deterministic, and was helpful for debugging, as later iteration/enumeration were changing order on each re-run.
There was a problem hiding this comment.
Just to be conservative, can you revert this? I don't know what the performance costs of this are for large graphs, and I don't trust our benchmarks to pick them up.
|
there are a few whiteline changes the pre commit hooks does. Not sure why, new version of black ? |
| assert_eq(z, x) | ||
|
|
||
|
|
||
| def test_blockwise_1_in_shape_I(): |
There was a problem hiding this comment.
@mrocklin can you confirm that these tests look correct to you? I still struggle with the blockwise syntax.
There was a problem hiding this comment.
I can also try to be clearer and in test_f make sure that the shape of a and b are compatible and can be broadcasted together.
There was a problem hiding this comment.
Sorry for the delay here. I probably won't get to this until this weekend.
TomAugspurger
left a comment
There was a problem hiding this comment.
@Carreau sorry for the delay. I'm going to trust that this doesn't break anything since our tests pass. If you could make the one requested change I'll get this merged.
dask/blockwise.py
Outdated
| assert set(numblocks) == block_names | ||
|
|
||
| dummy_indices = all_indices - set(out_indices) | ||
| dummy_indices = sorted(all_indices - set(out_indices)) |
There was a problem hiding this comment.
Just to be conservative, can you revert this? I don't know what the performance costs of this are for large graphs, and I don't trust our benchmarks to pick them up.
This is an attempt at trying to understand and fix issue dask#6340, this part of the codebase is relatively complex, so it is likely that the fix is incorrect/incomplete. Right now when we look at whether chunks should be concatenated. If so we avoid generating the extra dummy [0] which are of help with broadcasting as otherwise multiple copies of the arrays would be concatenated.
No problem, |
|
TravisCI finished, just didn't report back. Thanks @Carreau! |
This is an attempt at trying to understand and fix issue #6340,
this part of the codebase is relatively complex, so it is likely that
the fix is incorrect/incomplete.
Right now when we look at whether chunks should be concatenated. If so
we avoid generating the extra dummy [0] which are of help with
broadcasting as otherwise multiple copies of the arrays would be
concatenated.
black dask/flake8 dask