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

Bshuf_decompress_zstd_block consistent with bshuf_decompress_lz4_block #115

Merged
merged 4 commits into from
Feb 25, 2022

Conversation

fleon-psi
Copy link
Contributor

There is currently wrong return value of bshuf_decompress_zstd_block leading to issues with decompression, simple compress/decompress test is giving wrong result.

Signed-off-by: Filip Leonarski filip.leonarski@psi.ch

… bshuf_decompress_lz4_block

Signed-off-by: Filip Leonarski <filip.leonarski@psi.ch>
@jrs65
Copy link
Collaborator

jrs65 commented Feb 25, 2022

@fleon-psi thanks! Do you know what conditions it needs for the compress/decompress cycle to fail? I would have thought this would be caught by the unit tests, so at the very least I'd like to modify/add them to catch this one.

@jrs65
Copy link
Collaborator

jrs65 commented Feb 25, 2022

Ah, I think I see why it's not caught. Our unit tests only test zstd via the HDF5 filter pipeline, but the filter pipeline doesn't check the return value from bshuf_compress_zstd_block as thoroughly as the decompress_zstd routine in bitshuffle.ext does.

@fleon-psi
Copy link
Contributor Author

fleon-psi commented Feb 25, 2022

I use a following Catch2 unit test internally in my code and it triggers failure on REQUIRE(decomp_out == comp_out) assertion:

TEST_CASE("Bitshuffle_ZSTD","[ZSTD]") {
std::vector<int32_t> image(5121024 * sizeof(int32_t));
std::vector compressed(bshuf_compress_zstd_bound(512
1024, 4, 0));
std::vector<int32_t> decompressed(5121024 * sizeof(int32_t));
for (int i = 0; i < 512
1024; i++) image[i] = i;

int64_t comp_out = bshuf_compress_zstd(image.data(), compressed.data(), RAW_MODULE_SIZE, 4, 0,0);
REQUIRE(comp_out > 0);

int64_t decomp_out = bshuf_decompress_zstd(compressed.data(), decompressed.data(), 512*1024, 4, 0);
REQUIRE(decomp_out == comp_out);

REQUIRE(memcmp(image.data(), decompressed.data(), 512*1024*sizeof(uint32_t)) == 0);

}

@jrs65 jrs65 merged commit ac791b7 into kiyo-masui:master Feb 25, 2022
@jrs65
Copy link
Collaborator

jrs65 commented Feb 25, 2022

Okay, great. I've added some unit tests that catch this issue, so at least now we shouldn't have any regressions.

Thanks @fleon-psi !

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.

None yet

2 participants