-
Notifications
You must be signed in to change notification settings - Fork 43
SZ3.3 #94
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
SZ3.3 #94
Conversation
merge blockwise iterator to sz3.3 dev
# Conflicts: # include/SZ3/decomposition/InterpolationDecomposition.hpp
|
One comment: |
This PR merges new and updated test units for SZ3 modules including quantizer, lossless compression, encoder, and integration tests. The changes introduce comprehensive tests for module functionality and update header dependencies to utilize new memory utility functions.
Added unit tests for LinearQuantizer, Lossless compressors, and multiple encoders.
Introduced integration tests for SZ3 command-line functionalities including HDF5 repacking.
Updated header files to include MemoryUtil and added error checking in data decoding.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts: # .github/workflows/cmake.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the zstd integration to fetch and build against an external zstd release, and adds comprehensive SZ3 tests.
- Remove all local zstd source and header files
- Update CMakeLists to use FetchContent for zstd 1.4.5 and adjust include/source paths
- Add new unit and integration tests under
tools/test, including Python and C++ suites
Reviewed Changes
Copilot reviewed 102 out of 173 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/zstd/CMakeLists.txt | Switched to FetchContent-based zstd integration |
| tools/zstd/compress/.c/h, common/.c/h, etc. | Deleted local zstd implementation sources |
| tools/test/modules/*.cpp | Added module-level C++ tests for Quantizer, Lossless, Encoder |
| tools/test/integration/*.py | Added integration tests (SZ3 CLI, HDF5 repacking) |
| tools/test/deprecated/test_arithmetic_coding.cpp | Updated null-checks to nullptr in deprecated test |
|
|
||
| if os.path.exists(decompressed_file): | ||
| os.remove(decompressed_file) | ||
|
|
Copilot
AI
Jun 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function test_file never returns the parsed (abs_err, rel_err) values. Add a return abs_err, rel_err at the end so callers receive the expected results.
| return abs_err, rel_err |
| @@ -1,76 +1,91 @@ | |||
| include(FetchContent) | |||
Copilot
AI
Jun 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a cmake_minimum_required(VERSION 3.14) (or newer) and a project(...) declaration at the top to ensure FetchContent is available and project metadata is set correctly.
| data[i] = i % 100; | ||
| } | ||
|
|
||
| size_t data_len = 0, conf_len = 0; |
Copilot
AI
Jun 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variables data_len and conf_len are assigned but never used. Either remove them or assert their values to catch potential buffer issues.
* Modified pysz.py to change how data is transferred back to Python from C++ in both the compression and decompression functions. This results in a massive speed up of SZ3's Python API. * Integrated previous changes to pysz in the sz3.3_develop branch.
Major changes: