Uh oh!
There was an error while loading. Please reload this page.
Bump Numcodecs requirement to 0.6.2 - #352
Merged
Merged
Conversation
Previously MsgPack was turning bytes objects to unicode objects when round-tripping them. However this has been fixed in the latest version of Numcodecs. So correct this test now that MsgPack is working correctly.
As we already ensured the `chunk` is an `ndarray` viewing the original data, there is no need for us to do that here as well. Plus the checks performed by `ensure_contiguous_ndarray` are not needed for our use case here. Particularly as we have already handled the unusual type cases above. We also don't need to constrain the buffer size. As such the only thing we really need is to flatten the array and make it contiguous, which is what we handle here directly.
As both the expected `object` case and the non-`object` case perform a `reshape` to flatten the data, go ahead and refactor that out of both cases and handle it generally. Simplifies the code a bit.
As refactoring of the `reshape` step has effectively dropped the expected `object` type case, the checks for different types is a little more complicated than needed. To fix this, basically invert and swap the case ordering. This way we can handle all generally expected types first and simply cast them. Then we can raise if an `object` type shows up and is unexpected.
As Numcodecs now includes a very versatile and effective `ensure_bytes` function, there is no need to define our own in `zarr.storage` as well. So go ahead and drop it.
Make use of Numcodecs' `ensure_contiguous_ndarray` to take `ndarray` views onto buffers to be stored in a few cases so as to reshape them and avoid a copy (thanks to the buffer protocol). This ensures that datetime/timedeltas are handled by default. Also catches things like object arrays. Finally this handles flattening the array if needed. All-in-all this gets as close to a `bytes` object as possible while not copying and doing its best to preserve type information while constructing something that fits the buffer protocol.
Rewrite `buffer_size` to just use Numcodecs' `ensure_ndarray` to get an `ndarray` that views the data. Once the `ndarray` is gotten, all that is needed is to access its `nbytes` member, which returns the number of bytes that it takes up.
If the data is already a `str` instance, turn `ensure_str` into a no-op. For all other cases, make use of Numcodecs' `ensure_bytes` to aid `ensure_str` in coercing data through the buffer protocol. If we are on Python 3, then decode the `bytes` object to a `str`.
7 tasks
As Blosc got upgraded and it contained an upgrade of Zstd, the results changed a little bit for this example. So update them accordingly. Should fix the doctest failure.
alimanfoo
approved these changes
Dec 4, 2018
alimanfoo
left a comment
Member
There was a problem hiding this comment.
Thanks @jakirkham, all looks good.
jakirkham
commented
Dec 4, 2018
MemberAuthor
Looks like the line not covered is a fallback for when file removal fails. Given PR ( #327 ) obviates that, maybe we should merge that PR and drop that fallback. Thoughts? |
jakirkham
commented
Dec 4, 2018
MemberAuthor
Made PR ( #355 ) to simply ignore coverage on that group of lines. |
jakirkham
commented
Dec 4, 2018
MemberAuthor
Alright, think this is ready now. 😄 |
alimanfoo
commented
Dec 4, 2018
Member
Awesome, merging... |
7 tasks
jakirkham
commented
Dec 10, 2018
MemberAuthor
Missed a bit of code that could be simplified with |
This was referenced Dec 10, 2018
jakirkham
commented
Dec 10, 2018
MemberAuthor
Also dropping a workaround for older Numcodecs. ( #361 ) Please let me know if there are more of these. |
7 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to PR ( #347 )
Fixes#324
As there are some critical fixes and needed features in the latest Numcodecs, this bumps our lower bound to the latest version. Pulls most of the content from the aforementioned PR. Drops some commits that have been broken out into subsequent PRs to keep this focused on the Numcodecs upgrade. Does some refactoring and cleanup of internal functions thanks to utility functions now included in Numcodecs.
TODO:
tox -e docs)