-
-
Notifications
You must be signed in to change notification settings - Fork 329
Fix a bug when setting complete chunks #2851
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
Conversation
@@ -320,6 +309,20 @@ def _merge_chunk_array( | |||
for idx in range(chunk_spec.ndim) | |||
) | |||
chunk_value = chunk_value[item] | |||
if is_complete_chunk and chunk_value.shape == chunk_spec.shape: |
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.
this will fail for scalars, need a test for that too.
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.
Well it works for scalars, because chunk_value
is now NDBuffer which does have shape
. I added a test.
could you explain what the bug was, and how this PR fixes it? |
g = zarr.open_group("foo.zarr", zarr_format=3, mode="w")
a = g.create_array("bar", shape=(10,), chunks=(3,), dtype=int)
data = np.array([7, 8, 9])
a[slice(7, 10)] = data
np.testing.assert_array_equal(a[slice(7, 10)], data) I think it's broken for overwriting a complete last chunk that is smaller than the chunk size, AND where your setitem value is a size that is larger than that last-chunk-size. This also suggests that somewhere we can do
and it will assign |
Not saying it's related but reminds me of #2469 Thanks for investigating. |
1. Emphasize arrays of side > 1, 2. Emphasize indexing the last chunk for both setitem & getitem
OK I have updated the priorities of our property tests and it caught it immediately. Now we prioritize arrays of dim size >=3, and prioritize indexing the array near its end (so that we test handling the last chunk). We also don't test complicated attrs, array names, and long array paths in the indexing tests. I learnt a lesson. :) |
nice! |
c152ae5
to
ca6dcbc
Compare
f49c4e5
to
3f3e4f5
Compare
3f3e4f5
to
a13eec1
Compare
From manual inspection, the unit test in It succeeds on my macbook. Is someone able to debug this locally? |
@dcherian What do you expect to happen in that repeated index test? Isn't it ambiguous which values should be taken from the input if the output indexes overlap? It would be great for https://github.com/ilan-gold/zarrs-python if |
I am surprised it works, but zarr-python "sends the indexes to the chunk" so you get the last of the repeated values, numpy style. |
You know, it looks like the failing test is doing the opposite, and we get the first value:
|
Closes #2849
cc @ilan-gold
[Description of PR]
TODO:
docs/user-guide/*.rst
changes/