search

Blosc / bloscpack

Command line interface to and serialization format for Blosc
BSD 3-Clause "New" or "Revised" License
122 stars 27 forks source link

actually use the offset in CompressedFPSource #97

Open mmohrhard opened 4 years ago

esc commented 4 years ago

@mmohrhard hi and welcome to bloscpack. Thank you very much for your contribution and your efforts to improve bloscpack. I just had a look at the CI system and noticed, that the project has been dormant for some time. Hence the tests are not in good shape, they are testing older versions of Python and probably the APIs of the Numpy dependency have changed. This means, it is a bit tricky to work out why the tests are failing. I think it would be a good idea to improve the state of the test suite first before attempting to merge this fix.

esc commented 4 years ago

Hey, I fixed master, can you rebase this and force push please?

mmohrhard commented 4 years ago

I have pushed an updated patch but found one place where the current code is a bit ugly. I'm not too fond of the change in bloscpack/cli.py but it seems that there can be files without an offset header.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.003%) to 95.397% when pulling a6636de9f0b6fb0f44e9f255fe3a8425d2734e32 on mmohrhard:fix-offset-in-compressedfpsource into 2c0086b4f353b6635e4cf4f309debc36cb11477b on Blosc:master.

esc commented 4 years ago

From what I recall, it is possible to disable offsets in a file with the settings:

https://github.com/blosc/bloscpack#settings

This was done, in case people wanted smaller files with less over head but without the additional features that offsets enables.

esc commented 4 years ago

@mmohrhard thanks very much for contributing this, I have added it to the queue for review.

esc commented 3 years ago

@mmohrhard apologies for the delay, could you rebase this against current master. All CI issues have been fixed there. Thank you!