Skip to content

Add AsType codec from Zarr #12

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

Merged
merged 2 commits into from
Feb 21, 2017
Merged

Add AsType codec from Zarr #12

merged 2 commits into from
Feb 21, 2017

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Feb 19, 2017

Fixes https://github.com/alimanfoo/zarr/issues/102
Fixes https://github.com/alimanfoo/numcodecs/issues/7
Incorporates https://github.com/alimanfoo/zarr/pull/120

This is very much WIP. In particular, the tests need a bit of help getting cleaned up. Resolved.

This adds the AsType codec from Zarr to numcodecs.

@alimanfoo
Copy link
Member

Thanks @jakirkham. The astype module looks fine, just needs some work on the tests. There is a numcodecs.tests.common module which is supposed to provide some general support for encoding and decoding checks for any codec, it would be good to use as in the other tests modules. If you have time to spend that would be great, but just say if not, I'd be happy to merge as-is and then work on afterwards.

@jakirkham jakirkham changed the title WIP: Add AsType codec from Zarr Add AsType codec from Zarr Feb 21, 2017
@jakirkham
Copy link
Member Author

Thanks for the tips.

Went ahead and borrowed from the test suite for Delta in Numcodecs (much as was done in Zarr). Not sure if I have covered all the cases that were covered in Zarr, but don't have much more time to explore this near term. In any event, the tests seem to be passing and expanding the test suite should be straightforward now that there are some tests in place. 😉

Also have incorporated some changes to the representation of AsType that are the same as one's I'm proposing to Zarr. Hope that is ok. They were modeled after similar changes to Delta, which I'm guessing came about to handle Endianness. As such, it seems like an important change to include in both. Hope it is ok that I rolled it into here.

@alimanfoo
Copy link
Member

That's great, thank you. I'm happy to merge this and review the tests afterwards.

@jakirkham
Copy link
Member Author

SGTM

@alimanfoo alimanfoo merged commit 7a3a3b9 into zarr-developers:master Feb 21, 2017
@alimanfoo
Copy link
Member

Thanks very much.

@alimanfoo alimanfoo mentioned this pull request Feb 21, 2017
@jakirkham
Copy link
Member Author

Of course. Thought it was the least I could do after adding it to Zarr while in the midst of this transition.

Will try to go back and look at the tests in Zarr as well when I have more time.

@jakirkham jakirkham deleted the add_astype branch February 21, 2017 14:29
@alimanfoo alimanfoo modified the milestone: 0.1 Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants