Skip to content
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

Add lz4 compression type #258

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Add lz4 compression type #258

merged 3 commits into from
Jul 10, 2017

Conversation

vmarkovtsev
Copy link
Contributor

Fixes #254

Using python-lz4 here. I had to implement the custom stream wrapper as we read the file in blocks of fixed, small size and need to accumulate them. Unfortunately, lz4 does not contain the stream implementation yet.

Additional notes:

  • The default block size is in bytes now, not in numpy array rows (wtf? I guess it was a bug), and I set it to 4 MiB. Does not change anything for zlib and bzip2 (we use streams), but improves the efficiency of lz4.
  • I increased the data size in test_defragment(), otherwise bzip2 does not pass - likely because of the headr overhead.
  • I allowed compression objects to miss the flush() method.

@@ -90,3 +90,9 @@ def test_bzp2(tmpdir):
tree = _get_large_tree()

_roundtrip(tmpdir, tree, 'bzp2')


def test_lz4(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces lz4 as a test dependency. Can you please update setup.py so that this dependency is resolved and the test passes?

@vmarkovtsev vmarkovtsev force-pushed the lz4 branch 3 times, most recently from 1436354 to ed182c4 Compare July 8, 2017 08:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 94.229% when pulling 41e6398 on vmarkovtsev:lz4 into ab99df1 on spacetelescope:master.

@vmarkovtsev
Copy link
Contributor Author

@drdavella Done. I had to add lz4 to travis and appveyor configs. As for setup.py, added lz4 as the optional requirement in case "CI" env is defined (Travis). lz4 tests are skipped in case lz4 pkg is not installed.

@drdavella drdavella merged commit 9278462 into asdf-format:master Jul 10, 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.

Implement LZ4 compression of blobs
3 participants