Work around numpy 1.14 structured array changes #239
Conversation
|
Pushed a small change to tox config because coverage from py36-npy114 was overwriting py36-npy113 (where doctests are included). |
|
Hm, looks like still coverage issues. Will try something different. |
|
OK, coveralls looking happy. |
| return v | ||
| elif dtype.kind == 'V': | ||
| v = base64.standard_b64decode(v) | ||
| v = np.array(v, dtype=dtype.str).view(dtype)[()] |
There was a problem hiding this comment.
Clever! Are we doing any round trip tests of encoding/decoding and vice versa?
There was a problem hiding this comment.
This test covers encoding/decoding of metadata for a structured array. Probably could add a specific test for encoding/decoding structured array fill value.
There was a problem hiding this comment.
Looks good. Thanks for the pointer.
It couldn't hurt to have such a test if you want to add it. However it seems like this is already being tested by the array round trip. So don't think it is strictly necessary to test fill values specifically.
zarr/tests/test_core.py
Outdated
| # this does not raise with numpy 1.14 | ||
| # with pytest.raises(ValueError): | ||
| # # dodgy fill value | ||
| # self.create_array(shape=a.shape, chunks=2, dtype=a.dtype, fill_value=42) |
There was a problem hiding this comment.
Maybe we can add a version check with this test? Also do we know why this doesn't raise with NumPy 1.14?
There was a problem hiding this comment.
Numpy 1.14 is happy with a fill value like 42 for a structured array, it actually fills every field with the value, whereas numpy 1.13 barfs. I thought about adding a version check but then thought maybe that was overcomplicating, happy to add one if you think it's worth it.
There was a problem hiding this comment.
Interesting. No strong opinions from me.
That said, if we think this test is no longer worthwhile, we should delete it instead of commenting it. Commented code has a way of sticking around unaddressed. It's much easier to add a new PR marked WIP readding this code and it has a better chance of being addressed that way (if needed).
| # https://github.com/alimanfoo/zarr/pull/172#issuecomment-343782713 | ||
|
|
||
| if fill_value == 0: | ||
| fill_value = '' |
There was a problem hiding this comment.
Why is this no longer needed?
There was a problem hiding this comment.
This is now covered by this branch.
There was a problem hiding this comment.
Ah sorry. Overlooked that it is handled above generally. Thanks for the pointer. Seems fine.
|
Thanks for tackling this @alimanfoo! Looks pretty good to me. :) Had a couple of questions above mainly to make sure we are testing the round trip of encoding/decoding fill values and about some code that seems to have been dropped. |
|
Thanks @jakirkham for the review. I've deleted the commented code section, I don't think it's necessary. I'll merge and roll another release. |
|
Thanks @alimanfoo. |
This PR adds testing with numpy 1.14 and deals with some compatibility issues around fill values for structured arrays. Resolves #222, resolves #238.
TODO:
tox -e py36orpytest -v --doctest-modules zarr)tox -e py27orpytest -v zarr)tox -e py36orflake8 --max-line-length=100 zarr)tox -e py36orpython -m doctest -o NORMALIZE_WHITESPACE -o ELLIPSIS docs/tutorial.rst)tox -e docs)